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

[uss_qualifier] Clean up artifacts #282

Merged
merged 4 commits into from
Oct 23, 2023

Conversation

BenjaminPelletier
Copy link
Member

@BenjaminPelletier BenjaminPelletier commented Oct 21, 2023

This PR attempts to clean up artifact generation for uss_qualifier.

All of the artifacts for a particular test run are confined to a single folder by placing a single output_path field on the ArtifactsConfiguration. This cleans up the output folder substantially. Each individual artifact type is modified slightly to just specify a name within the output path rather than specifying the full output path (when applicable); artifacts that only have one way to be generated are assigned static names (e.g., report.json, report.html, sequence).

Two artifact types are removed: graph visualization and tested roles. These artifacts did not seem to be providing much value and they can be retrieved from the commit history if they ever need to be revived. In the mean time, removing them reduces maintenance burden.

Report redaction is now specified on a per-artifact basis, and the redacted report is provided by default to artifact generators that shouldn't need full details (tested requirements, templated reports).

The GitHub Pages index is updated to provide links to more of the outputs.

External report*.json verification is removed from run_locally.sh as the report validation in #257 should have removed it (probably was un-removed as part of a suboptimal merge).

A few smaller adjustments are made to various artifacts -- "report" is clarified to "raw_report", multiple tested requirements configurations are now possible.

Note that these changes are not backwards-compatible with the current configuration format, but I think this is acceptable as we are still in pre-v1.0.0 state and it is better to reach an optimal v1.0.0 format sooner than ease the configuration upgrade burden of alpha adopters.

@BenjaminPelletier BenjaminPelletier marked this pull request as ready for review October 21, 2023 20:52
Copy link
Contributor

@barroco barroco left a comment

Choose a reason for hiding this comment

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

I am investigating a report with non redacted token.

Copy link
Contributor

@barroco barroco left a comment

Choose a reason for hiding this comment

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

After a second read and reviewing the produced artifacts, it seems tokens are not redacted.
Occurence of the issue can be found in uspace/sequence/s8.html. Please see the comment below.

path = os.path.join(config.artifacts.output_path, "report.json")
logger.info(f"Writing raw report to {path}")
raw_report = config.artifacts.raw_report
report_to_write = redacted_report if _should_redact(raw_report) else report
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the configuration object should be passed in _should_redact instead of the report itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

_should_redact is looking for an object that may have a redact_access_tokens attribute which should match config.artifacts.raw_report (what raw_report is assigned the line above). I'll take a look at s8 for uspace.

Copy link
Contributor

@barroco barroco Oct 23, 2023

Choose a reason for hiding this comment

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

Oh. sorry just realized that we redact tokens by removing the signature. Everything is ok. Somehow, I missed the end of the token which is correctly redacted.

Copy link
Member Author

@BenjaminPelletier BenjaminPelletier Oct 23, 2023

Choose a reason for hiding this comment

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

Hmm, could you elaborate on where you're seeing unredacted tokens? I downloaded the CI run report artifacts and the first few tokens appear to be redacted (redacted means that the third/last part of the token is replaced with REDACTED, not that the token is removed entirely) in uspace/sequence/s8.html. The raw uspace report.json also seems to have REDACTED tokens (per spot check).

@BenjaminPelletier BenjaminPelletier merged commit 726592d into interuss:main Oct 23, 2023
8 checks passed
@BenjaminPelletier BenjaminPelletier deleted the artifact-cleanup branch October 23, 2023 17:26
github-actions bot added a commit that referenced this pull request Oct 23, 2023
* Clean up artifacts

* Fix dss_probing artifacts

* Fix shell lint 726592d
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.

2 participants