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

feat(ado-ext-telemetry): Embed upload artifact into task #1085

Merged
merged 12 commits into from Mar 23, 2022

Conversation

brocktaylor7
Copy link
Contributor

Details

This removes the need to have a separate publish task for the report artifacts in the azure-pipelines.yml. It is using the special logging commands to upload an artifact in ADO pipelines. It also updates the documentation accordingly.

I ran pipelines for Windows, Ubuntu, and Mac to ensure operability across operating systems. I also ran a test pipeline that still had the publish step defined in the yaml file. It fails with an error stating that the artifact already exists.

Motivation

Feature work, partially addressing #1063

Context

I initially attempted to use the uploadArtifact method defined in the azure-pipelines-task-lib/task library only to find out that under the hood it uses the special logging command via a console.log call. We are capturing and modifying the stdout, and anything that uses a console.log receives a ##debug prefix. Once I realized that the underlying mechanism was to use the logging commands, I switched to using the logging command directly.

Pull request checklist

  • [n/a] Addresses an existing issue: Fixes #0000
  • Added relevant unit test for your changes. (yarn test)
  • Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
  • Ran precheckin (yarn precheckin)
  • (after PR created) The Accessibility Checks (pull_request) check should fail. All other checks should pass.

@brocktaylor7 brocktaylor7 requested a review from a team as a code owner March 10, 2022 22:36
@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2022

Codecov Report

Merging #1085 (c1e426f) into main (fc1a26c) will increase coverage by 0.23%.
The diff coverage is 100.00%.

❗ Current head c1e426f differs from pull request most recent head ba4dd78. Consider uploading reports for the commit ba4dd78 to get more accurate results

@@            Coverage Diff             @@
##             main    #1085      +/-   ##
==========================================
+ Coverage   89.65%   89.89%   +0.23%     
==========================================
  Files          54       54              
  Lines        1286     1316      +30     
  Branches      151      156       +5     
==========================================
+ Hits         1153     1183      +30     
  Misses        133      133              
Impacted Files Coverage Δ
...tension/src/output-hooks/ado-stdout-transformer.ts 93.10% <ø> (ø)
...ss-reporter/console/ado-console-comment-creator.ts 95.91% <100.00%> (+1.47%) ⬆️
...s/ado-extension/src/task-config/ado-task-config.ts 100.00% <100.00%> (ø)
...d/src/console-output/result-console-log-builder.ts 100.00% <100.00%> (ø)
...ackages/shared/src/logger/console-logger-client.ts 100.00% <100.00%> (ø)
packages/shared/src/logger/logger.ts 100.00% <100.00%> (ø)
...es/shared/src/mark-down/result-markdown-builder.ts 100.00% <100.00%> (ø)

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 fc1a26c...ba4dd78. Read the comment docs.

ThanyaLeif
ThanyaLeif previously approved these changes Mar 10, 2022
Copy link
Contributor

@ThanyaLeif ThanyaLeif left a comment

Choose a reason for hiding this comment

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

Nicely done!

@brocktaylor7 brocktaylor7 added the pr: DO NOT MERGE Dont merge this PR. label Mar 11, 2022
@brocktaylor7 brocktaylor7 requested review from dbjorge and removed request for dbjorge March 21, 2022 21:19
@brocktaylor7 brocktaylor7 dismissed ThanyaLeif’s stale review March 21, 2022 21:21

Many changes made since initial review :)

@brocktaylor7 brocktaylor7 removed the pr: DO NOT MERGE Dont merge this PR. label Mar 21, 2022
"label": "Artifact Name",
"required": false,
"defaultValue": "accessibility-reports",
"helpMarkDown": "Name of the report artifact to be uploaded to the build."
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It might be good to be explicit about how this interacts with uploadResultsAsArtifact by adding a visibleRule for pipeline UX users and a Ignored if uploadResultAsArtifact is false. to the helpMarkdown for yaml users

Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

LGTM modulo the minor suggestion for the artifactName settings in task.json

@brocktaylor7 brocktaylor7 reopened this Mar 23, 2022
@brocktaylor7 brocktaylor7 merged commit 2994b6c into main Mar 23, 2022
@brocktaylor7 brocktaylor7 deleted the brocktaylor/ado-artifact-upload branch March 23, 2022 17:50
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

5 participants