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.

Sign up for GitHub

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

Jump to bottom

Fix & unit test awscli version comparison function #39

Merged
merged 4 commits into from
Mar 4, 2020
Merged

Fix & unit test awscli version comparison function #39

merged 4 commits into from
Mar 4, 2020

Conversation

pda
Copy link
Contributor

@pda pda commented Mar 3, 2020

The previous implementation was returning incorrect results.

e.g. it claimed that 1.11.40 >= 1.17.10, because;

  • 1 is not greater than 1
  • and 11 is not greater than 17
  • but, 40 is greater-than-or-equal-to 10; QED.

This PR introduces passing unit tests, several of which were failing against the previous implementation:

 ✓ version_a_gte_b: version compare: equal
 ✓ version_a_gte_b: version compare: simple major version success
 ✓ version_a_gte_b: version compare: simple major version failure
 ✓ version_a_gte_b: simple minor version success
 ✓ version_a_gte_b: simple minor version failure
 ✓ version_a_gte_b: simple patch version success
 ✓ version_a_gte_b: simple patch version failure
 ✓ version_a_gte_b: smaller major but bigger minor; failure
 ✓ version_a_gte_b: smaller minor but bigger patch; failure

@pda
Fix & unit test awscli version comparison function
9db1492 
The previous implementation was not correct.

e.g. it claimed that 1.11.40 >= 1.17.10, because;
- 1 is not greater than 1, and
- 11 is not greater than 17, but
- 40 is greater-than-or-equal-to 10, QED.

This PR introduces unit tests which failed against the previous
implementation and are now passing.
@pda pda requested a review from a team March 3, 2020 08:43
@pda
Move --no-include-email auto-detect inside login conditional
0c78b1a 
This makes it possible to source the environment file without executing
it, to unit test the version comparison function.
@pda
Copy link
Contributor Author

pda commented Mar 3, 2020

Actually I think this is incorrect too 🤦‍♂️

@pda
Copy link
Contributor Author

pda commented Mar 3, 2020

I think it's good now 😅

@pda pda mentioned this pull request Mar 3, 2020
Merged
1 task
toolmantim
toolmantim approved these changes Mar 3, 2020
View reviewed changes
Copy link
Contributor

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

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

Looks legit.

As we bump our minimum aws-cli version support for the plugin, might there be a way to simplify some of all this logic?

@pda
Copy link
Contributor Author

pda commented Mar 4, 2020

Yeah that would be nice, although we still need the version comparison for the new awcli v2 breaking changes.

@pda pda merged commit 68d338d into master Mar 4, 2020
@pda pda deleted the fix-version-compare branch March 4, 2020 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toolmantim toolmantim toolmantim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pda @toolmantim