Add pattern matching support for sketches report artifact names #78
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously the code for assembling the ZIP file path was duplicated.
The GitHub API is used to discover the sketches report workflow artifacts. The artifact archive is then downloaded from the URL provided by the API response. Previously, the artifact discovery methods only returned the download URL. The API response also contains the somewhat useful information of the artifact name. Previously this information was discarded because the name is already available indirectly from the sketches-reports-source input value. However, that will no longer be the case once pattern matching support for the sketches-reports-source input is added. The code is refactored to pass the full data object received from the API for the artifact between the functions. This gives direct access to the exact artifact name.
The sketches report workflow artifact is downloaded to a temporary folder and extracted. This requires working with paths. The pathlib module provides convenient and safe path handling and so should be used whenever there is significant involvement of paths in code.
The code iterates through the folder containing the sketches reports. The iteration code returns the full path to each sketches report file. However, for some reason that path was joined to itself when it was passed to `open`. Fortunately the pathlib module is smart enough to handle the joining of two identical absolute paths so this didn't cause a problem but it is pointless and confusing so the redundant path joining code is removed.
Previously, the test_get_artifact unit test was configured to provide coverage for the normal operation of the get_artifact function as well as its behavior when unable to download the workflow artifact. The code used to test the normal operation was almost completely unnecessary for testing the failure condition and the code for adjusting the test behavior depending on which conditions were intended to be covered added significant complexity. Since the failure is expected to occur under normal operating conditions due to transient network outages, it is worth continuing to provide coverage for this. So the chosen approach to providing coverage without negatively impacting the test code is to move it to a dedicated test.
Background ---------- The action was originally designed for a use case where there will only be a single sketches reports workflow artifact. At that time, this was possible even in cases where a job matrix was used to generate multiple parallel jobs to compile a set of sketches for an array of boards because the "actions/upload-artifact" GitHub Actions action allowed a single artifact to be uploaded to multiple times (one upload for the sketches report file of each of the matrix jobs). Uploading multiple times to a single artifact is prohibited starting from the recently released version 4.0.0 of the "actions/upload-artifact" action. It now becomes necessary for each of the sketch compilation jobs to produce a separate artifact, and for the "arduino/report-size-deltas" action to be able to consume the reports from multiple artifacts. Changes ------- Artifact Name Pattern Support ----------------------------- The most convenient way for the user to configure the action to identify the artifacts containing sketches reports is through an artifact name matching pattern. This will allow the user to avoid the need to maintain a list of explicit artifact names in the "arduino/report-size-deltas" workflow that can change over time as coverage for various boards is added to the sketch compilation workflow. The pattern is set via the action's `sketches-reports-source` input. NOTE: pattern support is only added for the use case where the action is ran from a scheduled workflow and the action's `sketches-reports-source` input is defining the artifact names to download. In the use case where the user is running the "arduino/report-size-deltas" action from the same workflow that does the sketch compilations, the action's `sketches-reports-source` input continues to be used to define the explicit path to the folder that contains the sketches report files. Pattern support was not added in this use case because there is no need for or benefit from such a feature. Recursive Sketches Reports Path Processing ------------------------------------------ In the use case where the user is running the "arduino/report-size-deltas" action from the same workflow that does the sketch compilations, the action's `sketches-reports-source` input is used to define the path to the folder that contains the sketches report files. In the common use pattern where workflow artifacts are used to transfer the sketches report files from a job matrix that compiles the sketches for multiple boards to the job with the `arduino/report-size-deltas` step, the "actions/download-artifact" action will be used to download the artifacts into the runner workspace, using that action's artifact name pattern matching capability. By default, the artifacts will each be downloaded to a dedicated folder under the folder specified via the "actions/download-artifact" action's `path` input. For this reason, it will be convenient for the "arduino/report-size-deltas" action to collect sketches reports recursively from each of the subfolders of the folder specified via the `sketches-reports-source` input. The "actions/download-artifact" action does provide a `merge-multiple` input that could be used to change that action's behavior to putting all the files in the root of the folder specified via its `path` input. However, requiring this of the "arduino/report-size-deltas" action users is less friendly, and is actually technically inferior in that it introduces the possibility of a filename collision in the event multiple artifacts contain a report file of the same name (as would occur if the sketch compilation job matrix was configured to use the same FQBN in multiple jobs). Backwards Compatibility ----------------------- Backwards compatibility for existing workflow is retained because the explicit artifact name in the `sketches-reports-source` input will also serve as a pattern to match the single artifact. If the user wants to update the "actions/upload-artifact" action in their sketch compilation workflow, and that workflow uploads multiple times to the same artifact, they will need to migrate the sketch compilation workflow to the new system, and might also update the "arduino/report-size-deltas" action's `sketches-reports-source` input to reflect that change, but that is a breaking change imposed by the "actions/upload-artifact" action bump, not by the "arduino/report-size-deltas" action bump.
per1234
added
type: imperfection
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #78 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 731 740 +9
=========================================
+ Hits 731 740 +9 ☔ View full report in Codecov by Sentry. |
henrygab
added a commit
to SimpleHacks/QDEC
that referenced
this pull request
May 10, 2024
henrygab
added a commit
to SimpleHacks/QDEC
that referenced
this pull request
May 10, 2024
henrygab
added a commit
to SimpleHacks/QDEC
that referenced
this pull request
May 10, 2024
* Adjust for the breaking changes caused by upgrade from `actions/upload-artifact@v3` to `actions/upload-artifact@v4` See also: arduino/report-size-deltas#78
henrygab
added a commit
to SimpleHacks/QDEC
that referenced
this pull request
May 10, 2024
* Adjust for the breaking changes caused by upgrade from `actions/upload-artifact@v3` to `actions/upload-artifact@v4` See also: arduino/report-size-deltas#78
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
The action was originally designed for a use case where there will only be a single sketches reports workflow artifact. At that time, this was possible even in cases where a job matrix was used to generate multiple parallel jobs to compile a set of sketches for an array of boards because the "actions/upload-artifact" GitHub Actions action allowed a single artifact to be uploaded to multiple times (one upload for the sketches report file of each of the matrix jobs).
Uploading multiple times to a single artifact is prohibited starting from the recently released version 4.0.0 of the "actions/upload-artifact" action. It now becomes necessary for each of the sketch compilation matrix jobs to produce a separate artifact, and for the "arduino/report-size-deltas" action to be able to consume the reports from multiple artifacts.
Changes
Artifact Name Pattern Support
The most convenient way for the user to configure the action to identify the artifacts containing sketches reports is through an artifact name matching pattern. This will allow the user to avoid the need to maintain a list of explicit artifact names in the "arduino/report-size-deltas" workflow that can change over time as coverage for various boards is added to the sketch compilation workflow. The pattern is set via the action's
sketches-reports-sourceinput.NOTE: pattern support is only added for the use case where the action is ran from a scheduled workflow and the action's
sketches-reports-sourceinput is defining the artifact names to download. In the use case where the user is running the "arduino/report-size-deltas" action from the same workflow that does the sketch compilations, the action'ssketches-reports-sourceinput continues to be used to define the explicit path to the folder that contains the sketches report files. Pattern support was not added in this use case because there is no need for or benefit from such a feature.Recursive Sketches Reports Path Processing
In the use case where the user is running the "arduino/report-size-deltas" action from the same workflow that does the sketch compilations, the action's
sketches-reports-sourceinput is used to define the path to the folder that contains the sketches report files.In the common use pattern where workflow artifacts are used to transfer the sketches report files from a job matrix that compiles the sketches for multiple boards to the job with the
arduino/report-size-deltasstep, the "actions/download-artifact" action will be used to download the artifacts into the runner workspace, using that action's artifact name pattern matching capability. By default, the artifacts will each be downloaded to a dedicated folder under the folder specified via the "actions/download-artifact" action'spathinput. For this reason, it will be convenient for the "arduino/report-size-deltas" action to collect sketches reports recursively from each of the subfolders of the folder specified via thesketches-reports-sourceinput.The "actions/download-artifact" action does provide a
merge-multipleinput that could be used to change that action's behavior to putting all the files in the root of the folder specified via itspathinput. However, requiring this of the "arduino/report-size-deltas" action users is less friendly, and is actually technically inferior in that it introduces the possibility of a filename collision in the event multiple artifacts contain a report file of the same name (as would occur if the sketch compilation job matrix was configured to use the same FQBN in multiple jobs).Backwards Compatibility
Backwards compatibility for existing workflow is retained because the explicit artifact name in the
sketches-reports-sourceinput will also serve as a pattern to match the single artifact.If the user wants to update the "actions/upload-artifact" action in their sketch compilation workflow, and that workflow uploads multiple times to the same artifact, they will need to migrate the sketch compilation workflow to the new system, and might also update the "arduino/report-size-deltas" action's
sketches-reports-sourceinput to reflect that change, but that is a breaking change imposed by the "actions/upload-artifact" action bump, not by the change made here to the "arduino/report-size-deltas" action.Additional Context
Resolves #55