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

feat(controller): Add support for ephemeral metadata on BlueGreen rollouts. Fixes #973 #974

Merged

Conversation

cooperbenson-qz
Copy link
Contributor

@cooperbenson-qz cooperbenson-qz commented Feb 5, 2021

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed the CLA.
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@CLAassistant
Copy link

CLAassistant commented Feb 5, 2021

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Feb 5, 2021

Codecov Report

Merging #974 (4055e00) into master (bed1261) will increase coverage by 0.01%.
The diff coverage is 70.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #974      +/-   ##
==========================================
+ Coverage   81.36%   81.38%   +0.01%     
==========================================
  Files         100      101       +1     
  Lines        8844     8862      +18     
==========================================
+ Hits         7196     7212      +16     
- Misses       1180     1181       +1     
- Partials      468      469       +1     
Impacted Files Coverage Δ
rollout/canary.go 83.55% <ø> (+3.55%) ⬆️
rollout/bluegreen.go 72.03% <33.33%> (-0.56%) ⬇️
rollout/ephemeralmetadata.go 65.95% <65.95%> (ø)
rollout/sync.go 71.74% <100.00%> (+0.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bed1261...4055e00. Read the comment docs.

@cooperbenson-qz
Copy link
Contributor Author

Current status:

  • Working on getting permission to sign the CLA from my org, should have that in an hour or so
  • I'm a little worried that I've made the e2e tests flaky. I had to add sleeps to get them to work which seems suspect to me
  • A documentation update is required, but I'm not sure how to go about it since the canary and blue-green terms are different and I don't want to add a bunch of redundant listing of terms

@jessesuen
Copy link
Member

This looks great!

Working on getting permission to sign the CLA from my org, should have that in an hour or so

DCO is sufficient. We've already moved off the CLA and will be disabling the bot.

I'm a little worried that I've made the e2e tests flaky. I had to add sleeps to get them to work which seems suspect to me

There is a ongoing issue with e2e tests flaking because of waiting for rollout spec update to be observed so I will ignore those failures.

A documentation update is required, but I'm not sure how to go about it since the canary and blue-green terms are different and I don't want to add a bunch of redundant listing of terms

We often refer to it as: desired (aka active/preview) vs. stable. For documentation, I'm thinking we can replace the use of canary with desired, along with an explanation that desired means canary or preview depending on the strategy.

@jessesuen
Copy link
Member

The change looks straight forward and pretty much good to go. The e2e tests are not your fault so I think the only thing left to do is to update documentation.

Please add in the documentation markdown, a note saying that this feature is a v1.0 feature so that < 0.10 users won't be confused when it doesn't work. e.g.:

!!! important

    Blue-green ephemeral metadata available in v1.0

Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
@cooperbenson-qz
Copy link
Contributor Author

Thanks for the suggestion for the documentation. Just pushed the update to the relevant docs page and took the opportunity to fix the link to USERS.md

Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
Signed-off-by: cooperbenson-qz <cooper.benson@quizlet.com>
@sonarcloud
Copy link

sonarcloud bot commented Feb 8, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
9.3% 9.3% Duplication

Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

Great job and appreciate the e2e test for this!

@jessesuen jessesuen merged commit 4e76b9c into argoproj:master Feb 10, 2021
@bobbyvacco
Copy link

The change looks straight forward and pretty much good to go. The e2e tests are not your fault so I think the only thing left to do is to update documentation.

Please add in the documentation markdown, a note saying that this feature is a v1.0 feature so that < 0.10 users won't be confused when it doesn't work. e.g.:

!!! important

    Blue-green ephemeral metadata available in v1.0

is this meant to say available in v.0.10?

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.

None yet

5 participants