Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for basic version requirements #63

Closed

Conversation

jacobbednarz
Copy link
Collaborator

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.

  • If you find a .iamy-version file, use the version defined in it. If
    not, continue on.
  • If the .iamy-version file contains a version number, compare it to
    the local version and ensure that the version in use meets the minimum
    requirements to operate. If it doesn't, inform the user of the
    discrepancy and what is required.

#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

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.

- If you find a `.iamy-version` file, use the version defined in it. If
  not, continue on.
- If the `.iamy-version` file contains a version number, compare it to
  the local version and ensure that the version in use meets the minimum
  requirements to operate. If it doesn't, inform the user of the
  discrepancy and what is required.

99designs#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 99designs#62
@jacobbednarz
Copy link
Collaborator Author

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.

iamy.go Show resolved Hide resolved
go.mod Show resolved Hide resolved
iamy.go Outdated Show resolved Hide resolved
iamy.go Outdated Show resolved Hide resolved
@jacobbednarz
Copy link
Collaborator Author

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

Copy link
Contributor

@patrobinson patrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to merge and release this as is, but prefer the logs go to debug so we're not too noisy

iamy.go Show resolved Hide resolved
iamy.go Show resolved Hide resolved
iamy.go Show resolved Hide resolved
@jacobbednarz
Copy link
Collaborator Author

jacobbednarz commented Apr 13, 2020

@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.

#63 (comment) provides some local testing steps if you'd like to confirm for yourself.

@patrobinson
Copy link
Contributor

patrobinson commented Apr 17, 2020

@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
Copy link
Collaborator Author

[...] 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
Copy link
Collaborator Author

jacobbednarz commented Apr 17, 2020

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
Copy link
Contributor

patrobinson commented Apr 17, 2020

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
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Member

mtibben commented Apr 17, 2020

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
Copy link
Collaborator Author

@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
Copy link
Contributor

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
Copy link
Member

mtibben commented Apr 22, 2020

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

@mipearson
Copy link

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
Copy link
Collaborator Author

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
Copy link
Contributor

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

@jacobbednarz
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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

@patrobinson
Copy link
Contributor

@mtibben what's your thoughts?

@patrobinson
Copy link
Contributor

ping @mtibben

@jacobbednarz
Copy link
Collaborator Author

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

@jacobbednarz jacobbednarz deleted the basic-version-checks branch March 9, 2021 04:09
andrewjhumphrey added a commit to envato/iamy that referenced this pull request May 6, 2021
andrewjhumphrey added a commit to envato/iamy that referenced this pull request May 7, 2021
patrobinson pushed a commit to envato/iamy that referenced this pull request May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: .iamy-version file
4 participants