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

Improve Conditions Returned By ApplicationGroup #313

Merged
merged 13 commits into from
Jun 16, 2021
Merged

Improve Conditions Returned By ApplicationGroup #313

merged 13 commits into from
Jun 16, 2021

Conversation

jonathan-innis
Copy link
Contributor

@jonathan-innis jonathan-innis commented Jun 13, 2021 •
edited by nitishm
Loading

Edit: @nitishm - Closes #328

  • Fixes the ApplicationGroup status conditions by returning the following condition types:
Ready
  - Progressing
  - Succeeded
  - ChartPullFailed
  - TemplateGenerationFailed
  - Terminating
ForwardWorkflowSucceeded
  - Progressing
  - Succeeded
  - Failed
ReverseWorkflowSucceeded
  - Progressing
  - Succeeded
  - Failed
RollbackWorkflowSucceeded
  - Progressing
  - Succeeded
  - Failed
  • Added a rollback client for rollback workflow generation specific operations
  • Rollback now doesn't overwrite previous spec, the following will happen on a failed rollout:
    • ReadyCondition will become WorkflowFailed
    • Generation == ObservedGeneration
    • Generation != LastSucceededGeneration
    • Rollback workflow will start and will be tracked in RollbackWorkflowSucceeded condition

@mahalrs
Copy link
Contributor

mahalrs commented Jun 13, 2021 •
edited
Loading

@jonathan-innis Thanks for adding ambassador v6.7.9 to https://nitishm.github.io/charts. @jonathan-innis @nitishm Can you add ambassador v6.6.0 to https://nitishm.github.io/charts. It's required for some of the e2e tests.

ambassador v6.6.0 is here https://github.com/datawire/ambassador-chart/releases/tag/v6.6.0

@jonathan-innis
Copy link
Contributor Author

@jonathan-innis Thanks for adding ambassador v6.7.9 to https://nitishm.github.io/charts. @jonathan-innis @nitishm Can you add ambassador v6.6.0 to https://nitishm.github.io/charts. It's required for some of the e2e tests.

ambassador v6.6.0 is here https://github.com/datawire/ambassador-chart/releases/tag/v6.6.0

Done @mahalrs 😄 Thanks for the reminder

@jonathan-innis jonathan-innis requested a review from nitishm June 13, 2021 22:13
@jonathan-innis jonathan-innis marked this pull request as ready for review June 13, 2021 22:13
@jonathan-innis jonathan-innis mentioned this pull request Jun 13, 2021
Open
@jonathan-innis jonathan-innis force-pushed the better-conditions branch 2 times, most recently from 83374ea to b905adb Compare June 14, 2021 00:42
@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2021 •
edited
Loading

Codecov Report

Merging #313 (9122e1d) into main (9e6538d) will decrease coverage by 1.95%.
The diff coverage is 7.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #313      +/-   ##
==========================================
- Coverage   24.04%   22.09%   -1.96%     
==========================================
  Files          12       14       +2     
  Lines         786      878      +92     
==========================================
+ Hits          189      194       +5     
- Misses        591      678      +87     
  Partials        6        6
Impacted Files Coverage Δ
pkg/workflow/workflow.go 0.00% <0.00%> (ø)
pkg/workflow/workflow_forward.go 0.00% <0.00%> (-2.88%) ⬇️
pkg/workflow/workflow_reverse.go 0.00% <0.00%> (ø)
pkg/workflow/workflow_rollback.go 0.00% <0.00%> (ø)
pkg/workflow/templates.go 3.65% <3.65%> (ø)
controllers/appgroup_controller.go 77.46% <66.66%> (+8.97%) ⬆️

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 9e6538d...9122e1d. Read the comment docs.

nitishm
nitishm reviewed Jun 15, 2021
View reviewed changes
pkg/helpers/status.go Outdated Show resolved Hide resolved
@jonathan-innis jonathan-innis requested a review from nitishm June 15, 2021 22:47
nitishm
nitishm reviewed Jun 15, 2021
View reviewed changes
pkg/helpers/reconcile.go Outdated
@@ -220,7 +217,7 @@ func (helper *ReconcileHelper) reconcileApplications() error {

if err := scc.Validate(); err != nil {
ll.Error(err, "failed to validate application subchart for staging registry")
err = fmt.Errorf("failed to validate application subchart for staging registry : %w", err)
err = fmt.Errorf("failed to validate application subchart for staging registry")
Copy link
Contributor

@nitishm nitishm Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why are we no longer wrapping the error returned by Validate? I see it in more lines below.

nitishm
nitishm previously approved these changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@nitishm nitishm 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 to me 🚀
A qn regarding the error changes though - why are we not wrapping them?

@jonathan-innis
Copy link
Contributor Author

Looks good to me 🚀
A qn regarding the error changes though - why are we not wrapping them?

Let me actually change this, this is a good point. I want the log error and the user-returned condition error to be different though. Log error should be wrapped, user-friendly error shouldn't be

@jonathan-innis
Copy link
Contributor Author

@nitishm Added wrapping back, removed duplicate log lines though

@jonathan-innis jonathan-innis requested a review from nitishm June 16, 2021 04:50
nitishm
nitishm approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@nitishm nitishm left a comment

Choose a reason for hiding this comment

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

🚀 🚀

@nitishm nitishm merged commit 64ded4a into Azure:main Jun 16, 2021
@jonathan-innis jonathan-innis deleted the better-conditions branch June 16, 2021 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

Improve status conditions returned by ApplicationGroup
4 participants
@jonathan-innis @mahalrs @codecov-commenter @nitishm