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

Update artifact handling #80

Merged
merged 19 commits into from
Feb 9, 2024
Merged

Update artifact handling #80

merged 19 commits into from
Feb 9, 2024

Conversation

joernott
Copy link
Contributor

@joernott joernott commented Feb 7, 2024

This pull request updates our usage of upload-artifact and download-artifact to v4. They introduced a breaking change which makes artifacts immutable, so we can't add more files to an already created artifact, a feature I relied on quite heavily. These are the changes:

  • Actionlint complained about "number" in the inputs of dispatch_release.yml. I replaced that by string.
  • I replaced v3 with v4 for all occurrences of upload-artifact and download-artifact. in a lot of actions
  • Initially, we had an artifact named "logs" where all the logs were consolidated. In one of the later 3.x versions, I also added these logs to the artifacts which were created by every step. I now remove the functionality to add them to the consolidated "logs" archive. This will render the logfile.prefix setting obsolete.
  • Initially, I created the "testplan" cached object to contain the initially generated testplan and then I used the begin-report action to add headers for the slack and summary report to the same archive. This has been streamlined by adding a new parameter "files" to the action and populating it with the files I initially uploaded. This eliminates one upload-artifact call as we add these files with begin-report now.
  • The actions begin_report and append-report use unique artifact names starting with testplan- now. The action generate_report uses a new pattern matching feature to download all those artifacts matching testplan-* instead of the original "testplan" artifact. I am then creating a new "testplan" artifact from it afterwards and delete the testplan-* artifacts to clean up. The cleanup is still buggy, I have a ticket open with the author: Not all artifacts matching the glob are deleted GeekyEggo/delete-artifact#22)
  • Breaking change: Whenever we create a code coverage file for sonarcloud, we now need different names for every artifact. So, we need to define a different prefix for every phpunit and codeception run. That means in every test plan using sonarcloud, we need to set a unique value for coverage.prefix. To pick up all those coverage reports, the sonarcloud action now uses the coverage_artifact parameter to define the pattern by which we choose the artifacts. In the universal workflow, I configured 'coverage-reports-**-${{ steps.sonarcloud_testplan_name.outputs.matrix_suffix }}', so all these coverage.prefix entries must start with "coverage-reports-".
  • The output_artifact parameter of the son arcloud action was previously unused. I now use it to define the name for a consolidated artifact containing all reports. I also delete the artifacts which were consolidated.

@joernott joernott self-assigned this Feb 7, 2024
@@ -85,11 +86,22 @@ runs:
sed -e 's|${{ inputs.strip_path }}||' -i ${FILE}
COVERAGE_FILES="${COVERAGE_FILES},${FILE}"
diff -y --color=always "${FILE}.orig" "${FILE}" || true
Copy link
Contributor

Choose a reason for hiding this comment

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

here file is not removed after checking diff

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 helps with debugging. As we have to consolidate all those coverage-reports into one single artifact, we can have the modified versions alongside the unmodified files in that artifact.
Before, the coverage-reports artifact only contained the unmodified files as it was created by the test jobs before and we only unpacked it here. Now, that we have to create a single artifact and delete all those single files created by the previous jobs, we can as well add originals and patched files together and if someone wonders, why files are not covered, he can see what the pattern replacement did.

with:
name: testplan
name: testplan-000_header
Copy link
Contributor

Choose a reason for hiding this comment

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

here testplan name is not same as append_report

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for this whole change is that we can't have a single "testplan" artifact which is created once and then every job adds stuff to it. As I wrote in the explanation above, these artifacts are now immutable. They can't be changed any more. So we create a lot of small artifacts which all start with testplan- instead. In the generate-report step, we unpack all those testplan-* artifacts, generate our reports, pack everything together in the final "testplan" artifact and delete all the testplan-* artifacts as their content is now in the single testplan artifact.

uses: actions/checkout@v4

- name: 'clean_cache'
- name: 'Clean Cache'
if: ${{ steps.generate_report.outputs.overall_status == 'success' }}
uses: 'OXID-eSales/github-actions/clean_cache@v3'
Copy link
Contributor

Choose a reason for hiding this comment

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

here it is v3

@@ -102,7 +102,7 @@ runs:

- name: Upload Log
if: ${{ always() && steps.diff.outputs.skip == 'false' }}
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: ${{ inputs.logfile_artifact }}
Copy link
Contributor

Choose a reason for hiding this comment

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

this logfile_artifact is removed right

@@ -669,7 +681,7 @@ sonarcloud:
custom_script_container: ''

coverage:
prefix: *coverage_prefix
prefix: coverage-report
Copy link
Contributor

Choose a reason for hiding this comment

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

isin't coverage-reports-sonarcloud

@@ -622,7 +634,7 @@ runtest:
prefix: 'deprecated_tests_artifacts'
coverage:
path: 'source/deprecated_tests_coverage.xml'
prefix: *coverage_prefix
prefix: coverage-report-runtest
Copy link
Contributor

Choose a reason for hiding this comment

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

coverage-reports-runtest

@joernott joernott merged commit a612856 into release-v4 Feb 9, 2024
@joernott joernott deleted the update_artifact_handling branch February 9, 2024 12:14
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