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.

Sign up for GitHub

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

Combine 2 fixes so that enough checks pass to be able to merge #15771

Merged
merged 2 commits into from
Jan 23, 2025
Merged

Combine 2 fixes so that enough checks pass to be able to merge #15771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a106e8
remove old psycopg tar file that we do not use
thedoubl3j Jan 23, 2025
cfb8618
Use upload artifact v4
AlanCoding Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/actions/upload_awx_devel_logs/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ runs:
docker logs tools_awx_1 > ${{ inputs.log-filename }}
- name: Upload AWX logs as artifact
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: docker-compose-logs
name: docker-compose-logs-${{ inputs.log-filename }}
path: ${{ inputs.log-filename }}
28 changes: 19 additions & 9 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ jobs:

- name: Upload debug output
if: failure()
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: awx-operator-debug-output
path: ${{ env.DEBUG_OUTPUT_DIR }}
Expand Down Expand Up @@ -328,7 +328,7 @@ jobs:
token: ${{ secrets.CODECOV_TOKEN }}

# Upload coverage report as artifact
- uses: actions/upload-artifact@v3
- uses: actions/upload-artifact@v4
if: always()
with:
name: coverage-${{ matrix.target-regex.name }}
Expand Down Expand Up @@ -359,19 +359,29 @@ jobs:
- name: Upgrade ansible-core
run: python3 -m pip install --upgrade ansible-core

- name: Download coverage artifacts
uses: actions/download-artifact@v3
- name: Download coverage artifacts A to H
uses: actions/download-artifact@v4
with:
name: coverage-a-h
Comment on lines +362 to +365
Copy link
Member

@relrod relrod Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I dislike the "groups" being hardcoded here 😕 - when I wrote this I tried really hard to make sure there was only one place the groups had to be defined (and that was in the CI matrix that actually splits up the groups.)

Copy link
Member Author

Choose a reason for hiding this comment

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

In prior state, to accomplish that, the coverage files were uploaded with the same name. Apparently, uploading with the same name resulted in merging them into the same folder, or using subfolders specified by path. This specific behavior was yanked from the Github action, so that name must always be unique. The need to download each became a problem with this change from the upload-artifact action. If this is not satisfied, it outright fails the check whenever you get to the step.

path: coverage

- name: Download coverage artifacts I to P
uses: actions/download-artifact@v4
with:
name: coverage-i-p
path: coverage

- name: Download coverage artifacts Z to Z
uses: actions/download-artifact@v4
with:
name: coverage-r-z0-9
path: coverage

- name: Combine coverage
run: |
make COLLECTION_VERSION=100.100.100-git install_collection
mkdir -p ~/.ansible/collections/ansible_collections/awx/awx/tests/output/coverage
cd coverage
for i in coverage-*; do
cp -rv $i/* ~/.ansible/collections/ansible_collections/awx/awx/tests/output/coverage/
done
cp -rv coverage/* ~/.ansible/collections/ansible_collections/awx/awx/tests/output/coverage/
Copy link
Member

Choose a reason for hiding this comment

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

this feels equivalent

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be good to have better documentation. But this is tied into the change in .github/actions/upload_awx_devel_logs/action.yml in a non-obvious way. Because we previously used the same name for upload of each artifact. It would then put each item into a different folder based on the path. Using the same name is exactly what the v3-v4 change prohibited. So now we upload based on different names, but the actions/download-artifact changes here put them into the same local folder. So the loop is removed. Each item has kind of like coverage "fragments". Were were combining the fragments into the same folder eventually anyway, so yeah the ultimate behavior should be the same.

Copy link
Member

Choose a reason for hiding this comment

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

this feels equivalent

It's not quite. The previous one iterates all directories starting with coverage- and copies all their files into the target. The new one just copies all the coverage-* directories into the target. i.e. the first copies all the inner files of the directories, the second copies the directories themselves.

Something like this might be equivalent if you really want to lose the loop:

cp -rv coverage/coverage-*/* ~/.ansible/collections/ansible_collections/awx/awx/tests/output/coverage/

Copy link
Member

Choose a reason for hiding this comment

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

But it sounds like this recursion is unnecessary now based on what @AlanCoding is saying. It sounds like there's no inner directory anymore?

cd ~/.ansible/collections/ansible_collections/awx/awx
ansible-test coverage combine --requirements
ansible-test coverage html
Expand Down Expand Up @@ -424,7 +434,7 @@ jobs:
done

- name: Upload coverage report as artifact
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: awx-collection-integration-coverage-html
path: ~/.ansible/collections/ansible_collections/awx/awx/tests/output/reports/coverage
Binary file removed licenses/psycopg-3.1.18.tar.gz
View file
Open in desktop
Binary file not shown.
Loading