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

Include term Attribute in Violations and Warnings Output #2045

Closed

Conversation

Arpit529Srivastava
Copy link

issue #1668

About

This Pull Request modifies the output format of the ec validate image command to include the term attribute in the violations and warnings reported through YAML or JSON formats. This change ensures that users can view important contextual information alongside each violation and warning without needing to provide the --info flag.

Before:

Violations and warnings were reported without the term attribute by default, making it harder for users to understand the context and significance of each issue.
Users had to remember to use the --info flag to access additional details, which could lead to confusion or oversight.

After:

Enhances Output: Automatically includes the term attribute in the JSON/YAML output for all violations and warnings, providing users with more context about each issue.
Improves Usability: Eliminates the need for the --info flag to access the term, making the output more user-friendly and informative by default.
Facilitates Filtering: Users can utilize the term for include/exclude causes more effectively, aiding in better decision-making during validation processes.

Signed-off-by: arpit529srivastava <[email protected]>
@Arpit529Srivastava
Copy link
Author

@simonbaird @ralphbean @caugello @zregvart

Please review this and add the label hacktoberfest-accepted and merge the pr. If any issue reach out to me.

@@ -217,9 +217,12 @@ func keepSomeMetadata(results []evaluator.Result) {
}
}

// keepSomeMetadataSingle retains only specific metadata keys in the result.
// **Updated to include "term" by default as per the acceptance criteria.**
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this line.

func keepSomeMetadataSingle(result evaluator.Result) {
for key := range result.Metadata {
if key == "code" || key == "effective_on" {
// Retain "code", "effective_on", and "term" keys
Copy link
Member

Choose a reason for hiding this comment

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

This comment doesn't seem very helpful. Let's drop it.

@lcarva
Copy link
Member

lcarva commented Oct 3, 2024

Thanks! Looks reasonable. Just a couple of minor comments. Could you also update the git commit with a more specific title and description?

Let's also update the tests as needed. See the Testing section in the README for how to run those locally if you want.

@Arpit529Srivastava
Copy link
Author

@lcarva please let me know if there is any issue or any changes that you want

@Arpit529Srivastava
Copy link
Author

Arpit529Srivastava commented Oct 3, 2024

Thanks! Looks reasonable. Just a couple of minor comments. Could you also update the git commit with a more specific title and description?

Let's also update the tests as needed. See the Testing section in the README for how to run those locally if you want.

@lcarva please add label hactoberfest and hactoberfest-accepted
sure will done

Copy link

codecov bot commented Oct 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.70%. Comparing base (a17b9de) to head (6178af8).
Report is 85 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2045   +/-   ##
=======================================
  Coverage   73.70%   73.70%           
=======================================
  Files          85       85           
  Lines        5561     5561           
=======================================
  Hits         4099     4099           
  Misses       1462     1462           
Flag Coverage Δ
generative 73.70% <100.00%> (ø)
integration 73.70% <100.00%> (ø)
unit 73.70% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
internal/output/output.go 89.32% <100.00%> (ø)

@Arpit529Srivastava
Copy link
Author

@lcarva can you please merge it

@lcarva
Copy link
Member

lcarva commented Oct 3, 2024

@Arpit529Srivastava, please see my comments and the failed test.

@simonbaird
Copy link
Member

+1 to fixing the commit message so it describes the change. https://cbea.ms/git-commit/ is a useful guide to follow.

@zregvart
Copy link
Member

zregvart commented Oct 4, 2024

/ok-to-test

@zregvart
Copy link
Member

zregvart commented Oct 4, 2024

Apart from a nicer commit message and making sure tests pass, we could also update the text output? A change to the internal/applicationsnapshot/templates/_results.tmpl would be needed.

@Arpit529Srivastava
Copy link
Author

Sure will do it by today

@lcarva
Copy link
Member

lcarva commented Oct 15, 2024

@Arpit529Srivastava, are you still interested in driving these changes?

@lcarva
Copy link
Member

lcarva commented Oct 31, 2024

Closing this due to lack of activity. Please, feel free to re-open it if you get back to it!

@lcarva lcarva closed this Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants