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

chore: Add IgnoreResourceStatusField field to DiffOptions #60

Closed
wants to merge 1 commit into from

Conversation

darshanime
Copy link
Member

Can be used to ignore /status of the objects.
See argoproj/argo-cd#3754

@codecov
Copy link

codecov bot commented Jun 20, 2020

Codecov Report

Merging #60 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #60   +/-   ##
=======================================
  Coverage   52.49%   52.49%           
=======================================
  Files          25       25           
  Lines        2585     2585           
=======================================
  Hits         1357     1357           
  Misses       1100     1100           
  Partials      128      128           
Impacted Files Coverage Δ
pkg/diff/diff.go 59.63% <ø> (ø)

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 fb2ec13...3814206. Read the comment docs.

@darshanime darshanime changed the title Add IgnoreResourceStatusField field to DiffOptions chore: Add IgnoreResourceStatusField field to DiffOptions Jun 20, 2020
@sonarcloud
Copy link

sonarcloud bot commented Jun 20, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

I think we should also provide a list of valid options for IgnoreResourceStatusField in something like an additional type, which in turn should also be used in the ArgoCD consumer. What do you think?

@darshanime
Copy link
Member Author

I thought about it, but then thought not all consumers of this library would want to abide by that classification. If it's a string field, they can use it in a way that fits their semantics.

What do you think?

@jannfis
Copy link
Member

jannfis commented Jun 22, 2020

Hm yes, that's a good point.

But now I'm also thinking about whether we should rather move the whole functionality into gitops-engine, instead of making it an argo-cd feature with a "dead" switch in gitops-engine. Or should we move the diff options (and its current features) to argo-cd?

@alexmt
Copy link
Contributor

alexmt commented Jun 23, 2020

Instead of adding an unused field, I would suggest adding data structure that inherits DiffOptions from gitops engine and adds IgnoreResourceStatusField.

@darshanime
Copy link
Member Author

This can be done in argo-cd, can I close this PR in that case?

@alexmt
Copy link
Contributor

alexmt commented Jun 24, 2020

sure @darshanime . lets close this PR and implement changes in Argo CD

@darshanime
Copy link
Member Author

cool, thanks for comments, closing

@darshanime darshanime closed this Jun 24, 2020
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

3 participants