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

Sync wave #1634

Merged
merged 445 commits into from
Jun 5, 2019
Merged

Sync wave #1634

merged 445 commits into from
Jun 5, 2019

Conversation

alexec
Copy link
Contributor

@alexec alexec commented May 21, 2019

No description provided.

@alexec alexec changed the title Sync wave Sync wave WIP May 21, 2019
@alexec
Copy link
Contributor Author

alexec commented Jun 3, 2019

Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

Added few comments. Looking good so far!

@@ -313,7 +317,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, revision st
}

diffResult := diffResults.Diffs[i]
if resState.Hook {
if resState.Hook || resource.Ignore(obj) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

} else {
fmt.Fprintf(w, "GROUP\tKIND\tNAMESPACE\tNAME\tSTATUS\tHEALTH\n")
}
func printAppResources(w io.Writer, app *argoappv1.Application) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like CLI won't print sync message any more. Can we restore this functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a tough one. A resource can be a hook, and a hook may have two messages (in edge cases, they can be part of two phases).

Options:

  • No message.
  • Only messages for non-hook resources.
  • Combine multiple messages into a summary message.

Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to combine messages into a summary message.

@alexec alexec requested review from jessesuen and alexmt and removed request for jessesuen June 4, 2019 16:28
Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

LGTM.

@alexec alexec merged commit 243378b into argoproj:master Jun 5, 2019
@alexec alexec deleted the sync-wave branch June 14, 2019 17:41
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.

3 participants