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

Handle tags returned in an unexpected order #3405

Merged
merged 5 commits into from
May 31, 2023

Conversation

SteveDMurphy
Copy link
Contributor

@SteveDMurphy SteveDMurphy commented May 31, 2023

Closes #3404

Code Changes

  • Add a failing test for tags that are returned in an unexpected order
  • Sort matched tags in descending order to ensure the increment is as expected

Steps to Confirm

  • Inspecting the matched tags (2.14.1a*) will show the two in tests as out of sequence. Reverting the last commit will show the test failing in that scenario

Pre-Merge Checklist

Description Of Changes

I did this manually last night to force 2.14.1a2 to push, then created an issue and PR to see if this was the right path forward.

As the tag names appear to follow a naming convention that will allow for this, sorting the tags in descending order will ensure we don't try to increment to a tag that already exists
@cypress
Copy link

cypress bot commented May 31, 2023

Passing run #2293 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 76fc550 into e68da2a...
Project: fides Commit: 5175e8fc55 ℹ️
Status: Passed Duration: 01:13 💡
Started: May 31, 2023 8:17 PM Ended: May 31, 2023 8:19 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@SteveDMurphy SteveDMurphy self-assigned this May 31, 2023
@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (58bea87) 87.13% compared to head (76fc550) 87.13%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3405   +/-   ##
=======================================
  Coverage   87.13%   87.13%           
=======================================
  Files         312      312           
  Lines       18659    18659           
  Branches     2377     2377           
=======================================
  Hits        16259    16259           
  Misses       1980     1980           
  Partials      420      420           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

this looks great, thank you @SteveDMurphy for making this more robust!

i'm pretty surprised that you were getting the tags returned out of order - it does indeed look like the 2.14.1a0 tag was created after the 2.14.1a1 tag - i'm guessing someone must have re-published the tag.

i assumed tags would be published in the right order and that they were basically immutable, but it's certainly possible to delete and re-add a tag. i guess i didn't expect anyone to do that 😄. regardless, there's no benefit in assuming this, and doing the more defensive, explicit sorting you've put in here is a good safety measure (that we clearly need!). so thank you again for the catch/fix!

just some minor comments, the gist of this looks good to me 👍

tests/nox/test_git_nox.py Outdated Show resolved Hide resolved
tests/nox/test_git_nox.py Outdated Show resolved Hide resolved
noxfiles/git_nox.py Outdated Show resolved Hide resolved
@adamsachs
Copy link
Contributor

oh also, i forgot to mention - we should make the same changes over on the fidesplus side! we've got the same nox commands over there, unfortunately we don't have a good way of sharing nox code between the two repos so we've gotta just c+p for now. hopefully the tagging code stabilizes after this change! 🤞

@SteveDMurphy
Copy link
Contributor Author

oh also, i forgot to mention - we should make the same changes over on the fidesplus side! we've got the same nox commands over there, unfortunately we don't have a good way of sharing nox code between the two repos so we've gotta just c+p for now. hopefully the tagging code stabilizes after this change! 🤞

Once we are good here I can pop a PR open over there as well, thanks for calling that out!

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

looks good on my end, thanks again for being proactive and making this fix!

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

looks good on my end, thanks again for being proactive and making this fix!

@SteveDMurphy SteveDMurphy marked this pull request as ready for review May 31, 2023 21:52
@SteveDMurphy SteveDMurphy merged commit 318e77f into main May 31, 2023
@SteveDMurphy SteveDMurphy deleted the SteveDMurphy-3404-alpha-tag-error branch May 31, 2023 21:52
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.

Unsorted tags casuing conflicts in nox -s "tag(push)"
2 participants