💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

There's a lot to digest in this feature (which is great!) so I'll start by focussing first on the interface and the overall design before diving into the implementation.

Do we want to keep the command structure as-is? This only applies to the logic of stack so I made it a sub-command, but I guess we do could do elastic-package profiles ...

I'd be okay with elastic-package profiles. If we ever need profiles for another command of elastic-package besides stack, that would give us the flexibility. If we go with elastic-package stack profiles now, I can see how that might box us into a corner and it might be awkward to get out of it later.

Also, do we want to lasso the deployer configs into this? This is limited to the stack config used by docker-compose, but there's no reason we can't expand it. Also, never used terraform and I don't have much experience with K8s in general, so I'm not sure if we want to touch those configs. I tried to make this fairly modular, so it shouldn't be too hard to expand the files managed by profiles.

I'm good with keeping this out of scope for now. As you said, as long as the design is fairly modular we can expand it as needed later.

There's also the issue of application_config, which I sort of left in. I'm not sure if we just want to flat-out remove the code that creates ~/.elastic-package/config.yml?

My understanding (@mtojek can correct me if I'm wrong) is that the application configs are the ones set by elastic-package itself at build time. I can imagine a user being able to override these locally via profiles (this PR's feature). WDYT of that?

is that the application configs are the ones set by elastic-package itself at build time.

@ycombinator I'm mostly looking at the code here: #275 which was basically a shim while we dealt with a bunch of elastic-agent bugs.

Hmm, okay, os.Read* is new to 1.16 since ioutil is getting deprecated. CI is using 15.8 and beats is 15.9. Not sure how to proceed here...

Okay, so adding .go-version doesn't seem to work. Not sure if we want to revert to the soon-to-be-depricated ioutil functions or what.

@fearful-symmetry Don't bother removing the deprecated ioutil functions in this PR. We have an issue to track this package's removal en masse across this codebase whenever we're ready to move to Go 1.16: #231. So we'll just sweep up any usages introduced in this PR at that time.

@mtojek that might work? A huge chunk of this code is basally reorganizing (see internal/locations) to get everything to play nice, so we're still gonna end up with a fairly large first PR and a bunch of tiny PRs after that. The two new libraries internal/profile and internal/location sort of need to happen together, and they're the biggest chunk of the whole thing.

Well, 30 files is definitely too much for a single PR and will probably slow us down with merging this in. If you are able to split this codebase more into features, so that we have 7-8 files per PR, then it will make reading this easier.

The two new libraries internal/profile and internal/location sort of need to happen together

I see your point, but we need to cut this into reviewable pieces. I don't mind if two libraries need to go together, but for me the most important part is value added to the codebase (new feature). Thus, I suggest to split it.

I see your point, but we need to cut this into reviewable pieces.

I definitely agree, I'm just worried about merging something half-broken.

I think the best way to do this would be like this:

1. internal/locations
2. profile/profile
3. Everything else.

That should work, as a good part of the PR is just the renames associated with creating the locations library. profile needs locations but locations should be able to stand on its own. How does that sound @mtojek ?

Ok, let's try such approach. I personally prefer to open pull requests which add a real value for the end user, not just a library or simple refactoring, but you're the driver here :) I'm looking forward to reviewing the first PR.

If it's easier for you to split this PR, you can keep open as a draft.

Ok, let's try such approach. I personally prefer to open pull requests which add a real value for the end user, not just a library or simple refactoring, but you're the driver here

Agreed, but I think it's the only way to break the whole feature addition down into manageable chunks, as 6-7 of the files are just changes associated with locations. @mtojek

I'd like to see this split into multiple PRs as well, even if it means the first PR or two are mostly refactoring / prep for later PRs that actually implement the profiles feature.

Alternatively, we also have the option of creating a feature branch and targeting the PRs to it initially if we don't want to land half-baked code into master directly.

@fearful-symmetry @mtojek Before we start breaking this up into smaller PRs and reviewing them, it would be good to settle on the overall design and interface for this feature. @fearful-symmetry raised some questions in #301 (comment) and I added some comments in #301 (comment).

Do either/both of you have anything to add? It would be good to reach some consensus on these points before we start reviewing smaller PRs. Otherwise we might merge a couple of initial PRs and then have to change course later, which wouldn't be a great use of time IMO.

Let me respond to raised questions too:

Do we want to keep the command structure as-is? This only applies to the logic of stack so I made it a sub-command, but I guess we do could do elastic-package profiles ...

This is a great question. We may leverage from keeping profiles independent from the stack only in the future. I would be for keeping it as a separate feature.

Also, do we want to lasso the deployer configs into this? This is limited to the stack config used by docker-compose, but there's no reason we can't expand it. Also, never used terraform and I don't have much experience with K8s in general, so I'm not sure if we want to touch those configs. I tried to make this fairly modular, so it shouldn't be too hard to expand the files managed by profiles.

I agree with Shaunak, let's not introduce additional complexity here. We can iterate on this later on, even leave it for further improvement if it becomes a problem (missing feature).

There's also the issue of application_config, which I sort of left in. I'm not sure if we just want to flat-out remove the code that creates ~/.elastic-package/config.yml?

This piece of code is a safety switch to be used in case of problems with stable SNAPSHOTs. If we know that the SNAPSHOT is faulty, we can mark a particular revision as the recommended one. Unfortunately it's required due to many drawbacks of current unified building process (one piece can block the entire snapshot).

Thanks for the updates @fearful-symmetry. Don't worry about the pace at which this PR is progressing. AFAIK we don't have an urgent demand for profiles (although they will solve some configuration problems rather nicely once ready) and I think it's more important to design this feature right than rush something out.

I tried out the latest set of changes and the contents under ~/.elastic-package/ look much cleaner to me now.

Moving on from the folder structure, I have a suggestion: could we rename the elastic-package profiles new sub-command to elastic-package profiles create? We have an elastic-package create subcommand so it would be nice to align the terminology.

I created a new profile foo. I didn't specify any --from argument when creating it so it got created from the default profile. When I list profiles I get this:

elastic-package profiles list
+---------+---------------------------+---------+---------+--------------------------------------------------+
|  NAME   |       DATE CREATED        |  USER   | VERSION |                       PATH                       |
+---------+---------------------------+---------+---------+--------------------------------------------------+
| default | 2021-05-20T15:29:25-07:00 | shaunak | d5152c1 | /Users/shaunak/.elastic-package/profiles/default |
+---------+---------------------------+         +         +--------------------------------------------------+
| foo     | 2021-05-20T15:35:16-07:00 |         |         | /Users/shaunak/.elastic-package/profiles/foo     |
+---------+---------------------------+---------+---------+--------------------------------------------------+

It would be nice if the user and version were repeated for the foo profile row as well. I believe this is the same problem as #360.

After creating a foo profile from the default profile, I now have two profiles. However, when I try to delete the default profile it won't let me. I understand why this is the case: because profile-aware commands like elastic-package stack need to default to using the default profile.

However, I wonder — for a follow up PR — if we should introduce a notion of a current profile, something that could be set via elastic-package profiles use <profile ID> or similar. Initially the default profile will be automatically set as the current profile. Creating a new profile would automatically set that one to be the current profile. It would not be possible to delete the current profile. Then, profile-aware commands like elastic-package stack would default to the current profile, unless a different one is explicitly specified via the --profile command. WDYT? If you like this idea, I'll file an enhancement issue for a follow up PR.

I'm a little concerned that a profile is simply a copy of all internal configuration and script files like healthcheck.sh.

One one hand it gives a user an incredible amount of control to do whatever they want, including shooting themselves in their feet. BTW, I'm less concerned about users making changes to their profiles and consequently shooting themselves in their feet because, presumably, a user could always use the default profile to achieve a working state while they debug their profile's changes.

On the other hand, it exposes a lot of internals of elastic-package implementation, which means now every file that's part of a profile is now part of the public interface of elastic-package. If we (elastic-package maintainers) wanted to change a file, say healthcheck.sh, that's part of a profile but the user has created a new profile on their system with this file in it, and then they upgrade elastic-package, would the newer version of elastic-package work with their profile's copy of the file?

Also, by implementing a profile as a copy of all internal configuration and script files like healthcheck.sh, we're implicitly requiring the user to understand how the elastic-package code uses these files. Maybe this is not such a huge mental burden for users, but it feels like a step away from an easier developer experience to me. But maybe I'm wrong about this, particularly if we think of the developers who'd create their own elastic-package profiles as more advanced anyway.

FWIW, I was imagining file(s) under a profile to be just properties/settings file(s), much like filebeat.yml or a kubeconfig file. Settings from such profile file(s) would be interpolated into actual, underlying configuration files and scripts that are part of the implementation of elastic-package. This minimizes the surface area of the elastic-package public interface, giving us (elastic-package maintainers) greater freedom to modify the internal configuration and script files as needed without having to worry too much about backwards compatibility with profiles created with older versions of elastic-package.

if we should introduce a notion of a current profile, something that could be set via elastic-package profiles use or similar.

I was actually contemplating something like this earlier today, and figured it was outside the scope of the current PR. I think you're definitely right, we should have some some kind of "current" profile.

As far as what APIs we're exposing to the user via the profile system--that's something I was wondering about as well. On one hand, you're right, and we risk giving users a bit of a foot-cannon when they upgrade the tooling and their old profile doesn't work any more. On the other hand, I've had to do some "creative" hacks in the past when testing components of the system module, so I kind of developed this from a perspective of "the user should be able to spin up the stack in a way that's very different from what we anticipate." I should probably try to pull out the health check, since that seems a tad unnecessary.

I wonder if there's a clever way to manage files under the profile system. Version tagging? Some kind of merge function? Copy on Write?

Alright @ycombinator fixed the way elastic-package profiles list works, changed the command to create and moved out healthcheck.sh.

Created #361 for the follow up enhancement of current profiles.

Checked out the latest changes. The table produced by elastic-package profiles list looks good now, much more readable IMO. Thanks for pulling out healthcheck.sh too.

As far as what APIs we're exposing to the user via the profile system--that's something I was wondering about as well. On one hand, you're right, and we risk giving users a bit of a foot-cannon when they upgrade the tooling and their old profile doesn't work any more. On the other hand, I've had to do some "creative" hacks in the past when testing components of the system module, so I kind of developed this from a perspective of "the user should be able to spin up the stack in a way that's very different from what we anticipate."

Fair point, and I'll happily defer to you on the right level of interface exposure here as you are the most experienced user for this use case.

I wonder if there's a clever way to manage files under the profile system. Version tagging? Some kind of merge function? Copy on Write?

Merging sounds fraught with other potential problems — like how to recover from conflicts?

By "version tagging" I assume you mean something like "this profile is compatible with versions X through Y of elastic-package" or something like that?

Similarly, could you elaborate on how copy-on-write could help?

Thanks for your patience with working through these issues. I love how powerful this feature is but I'm also glad we're taking the time to think through user experience for it!

Merging sounds fraught with other potential problems — like how to recover from conflicts?

@ycombinator I was mostly throwing ideas out there, most of which would require a certain bit of over-engineering. The simplest would be some kind of version tagging attached to managed files. If we push an update that requires a new snapshot.yml or something, we tag that file with a new major version, and if it doesn't match the version the user has, we can warn them. Could also be expanded to the list command, and we could perhaps tell a user that a given profile will no longer work.

Version conflicts might be annoying but on the hand the user might have messed up with their config so much that even the smartest handling won't solve conflicts. I'm fine with postponing this for later.

@mtojek that was the directory structure I tried to implement, I think I was a tad confused with what you were suggesting. So we just want each profile to have it's own stack where we place the pre-existing files, then? The healthcheck.sh is now in the global ~/.elastic-package/stack` based on a conversation with @ycombinator, where we decided that the healthcheck files are perhaps too closely tied to the implementation, and thus can break easily, and there's probably few reasons a user will need to change it.

@mtojek So we just want each profile to have it's own stack where we place the pre-existing files, then?

Correct. There would be also a global "stack" directory for built packages in "development".

The healthcheck.sh is now in the global ~/.elastic-package/stack` based on a conversation with @ycombinator, where we decided that the healthcheck files are perhaps too closely tied to the implementation, and thus can break easily, and there's probably few reasons a user will need to change it.

Ok, it seems that I missed this discussion. I'm ok with leaving it as it's now - global healthcheck. My counter-argument is that's a bit scattered across two places (global + profile's). It's up to you to decide, Alex.

One last thought:

         default/
            stack/
               Dockerfile.package-registry
               kibana.config.yml
               healthcheck.sh # not sure why did you detached the healthcheck file. IMHO it's part of the stack too.
               <other stack files>

If we agree on preserving the layout above, will "profile.json" be present in the profile directory? e.g.:

profile_name/
    stack/
       <stack_files>
    profile.json

My counter-argument is that's a bit scattered across two places (global + profile's). It's up to you to decide, Alex.

It's a bit scattered, yah, but I think we should be selective about what we put under the profile system. As elastic-package grows, it's unlikely that every component of the docker orchestration will need to be tinkered with by users, and we risk increasing the number of components that might break when the user upgrades.

Either way, let me tinker with the file paths a bit more.

Alright, I think this looks a tad cleaner:

/home/alexk/.elastic-package/profiles/default
├── profile.json
└── stack
    ├── Dockerfile.package-registry
    ├── kibana.config.yml
    ├── package-registry.config.yml
    └── snapshot.yml

@ycombinator did you still want to approve this?

@fearful-symmetry I think you can proceed with this PR and dep change in integrations as Shaunak will not continue work on package tooling.

Glad to see this PR merged! I was on PTO all last week so ++ for not waiting for my review.