99designs / iamy

A cli tool for importing and exporting AWS IAM configuration to YAML files
MIT License
238 stars 24 forks source link

Add support for basic version requirements #63

Closed jacobbednarz closed 3 years ago

jacobbednarz commented 5 years ago

With any project, eventually you get to a stage where you have configuration defined that works on a specific version of your software. Upgrading between versions usually requires some effort but it's rolled out when it's needed. This becomes even more difficult working in a team where people can have different versions of the software that you use based on how often they like to update their systems.

To handle these scenarios, you introduce some sort of versioning that defines what versions you want the configuration to operate with and the users then need to meet those requirements.

This introduces a basic concept of the versioning for IAMy. The logic is pretty straight forward.

62 proposes quite a few more coditions to restrain the version however

I don't think that's required for majority of use cases. I think defining a minimum version covers most of the sharp edges we've encountered. But, we can always make this more perscriptive if needed.

Closes #62

jacobbednarz commented 5 years ago

If you'd like to test this locally, you can pull it down and try the following commands for the various options:

echo "2.1.1" > .iamy-version
go build -ldflags "-X main.Version=1.2.3" -o dev-iamy
./dev-iamy pull 
rm .iamy-version
go build -ldflags "-X main.Version=1.2.3" -o dev-iamy
./dev-iamy pull 
echo "2.1.1" > .iamy-version
go build -ldflags "-X main.Version=1.2.3" -o dev-iamy
./dev-iamy pull --debug

I purposefully use dev-iamy here to prevent any $PATH madness that might arise.

jacobbednarz commented 4 years ago

@mtibben Any objections from you on this one? I was bit by this again today (almost 12 months to the date of raising it šŸ˜‚ )

jacobbednarz commented 4 years ago

@patrobinson I've just gone through this again and it's already output as a debug statement when --debug is provided. The only thing that doesn't follow this is the failure at the end which we want to enforce if it gets there as the project and version mismatch.

https://github.com/99designs/iamy/pull/63#issuecomment-487438896 provides some local testing steps if you'd like to confirm for yourself.

patrobinson commented 4 years ago

@jacobbednarz sorry for taking such a long time to get back to you.

running a dev build of this presents a problem, Semver returns the default empty value 0.0.0 because v2.3.2-2-gf11beb2 is not a real semantic version so it fails. I wonder if we should ignore empty versions, warn about this or allow it to be ignored via a CLI flag.

jacobbednarz commented 4 years ago

[...] because v2.3.2-2-gf11beb2 is not a real semantic version so it fails

This is something we should probably fix. The semver specification does allow build metadata which seems like a good fix here and would be compatible. Something like v2.3.2+gf11beb2 seems reasonable or v2.3.2+2.gf11beb2 if that 2 is meaningful and would also address the issue of the comparison.

I had added the additional check for bypassing Rev = "dev" as it was the default value but didn't realise the Makefile also had another way.

jacobbednarz commented 4 years ago

Just above the build metadata section there is also a section on prerelease šŸ¤¦ This is almost what we have but need to make the formatting match the expected input and it should be šŸ‘Œ . Build metadata might be more appropriate as it's a sha but I'm not particularly phased.

patrobinson commented 4 years ago

The requirements on pre-release appear to allow our version format:

A pre-release version MAY be denoted by appending a hyphen and a series of dot separated identifiers immediately following the patch version. Identifiers MUST comprise only ASCII alphanumerics and hyphen [0-9A-Za-z-]

The issue is the v at the beginning is not supported by this library.

0.0.0 Invalid character(s) found in major number "v2"

When I try using golang.org/x/mod/semver then it behaves as expected. Can we switch to using this library?

https://play.golang.org/p/06p63JxVvwV

jacobbednarz commented 4 years ago

Ah gotcha, I totally missed the little trailing hyphen in there. I don't feel strongly about swapping the library in use but the return values for Compare are a bit cryptic.

func Compare(v, w string) int The result will be 0 if v == w, -1 if v < w, or +1 if v > w.

If it's just the "v" throwing this off, we can ignore for the purposes of the comparison.

patrobinson commented 4 years ago

I don't have a strong opinion, but I'd default to the golang module package due to the implied support of using a golang library.

mtibben commented 4 years ago

There have been a few requests around wanting to configure optional features

I wonder if we should consider creating a more general .iamy file with version + config?

jacobbednarz commented 4 years ago

@mtibben Considering the amount of time something like using a consistent version in a project has taken to get to this point (> 12 months), I'm hesitant to bring in more scope.

With the included examples, I am also wondering whether explicit command arguments are a better fit (something like --ignore-policies "s3,ecr"). I don't feel strongly about where they should live as I can see use cases for both.

patrobinson commented 4 years ago

I think it's tempting to combine these two requirements but I'm also tempted to get this particular functionality out and ship those seperately.

@jacobbednarz What can I do to help get this PR over the line?

mtibben commented 4 years ago

Playing devils advocate, do tools like https://github.com/asdf-vm/asdf fill this gap better?

mipearson commented 4 years ago

As an asdf user - I don't think so. This is something that needs to be controlled by the "owner" of the particular iamy workspace and enforced by the tool to be effective - whereas asdf is very much "opt in".

(also bitten by this quite a few times, has had team members bitten, work alongside jacob & pat)

jacobbednarz commented 4 years ago

You could use asdf for this and it could work in a similar fashion to what we have here. The drawback then becomes that people have to install and use asdf to maintain a safer approach to this tool. I don't know about others but I think that's a pretty tough sell. I'd be happy to supplement this withasdf support but I don't think it quite gets us all the way there in the same way this PR does by baking in version support.

patrobinson commented 4 years ago

@mtibben are you happy for this PR to get merged?

jacobbednarz commented 4 years ago

@patrobinson We still need to fix the version comparison. I'll look to either swap it out for stdlib today, or update the comparison to account for it.

jacobbednarz commented 4 years ago

Sorted, it will work for vX.X.X and X.X.X now

patrobinson commented 4 years ago

@mtibben what's your thoughts?

patrobinson commented 4 years ago

ping @mtibben

jacobbednarz commented 3 years ago

closing due to necromancy. if someone decides to persue this, feel free to cherry pick the commits from here.