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

CI: Add test for uploads with multiple labels #54

Merged
merged 3 commits into from
Jan 24, 2024
Merged

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Jan 24, 2024

Add test uploads for non-main label 'test' and for multiple labels in a comma seperated list to compliment the feature added in PR #47.

This is an example of things working as intended prior to the cleanup step removing the test package.

pre-clean-up

* Add test uploads for non-main label 'test' and for multiple labels in
  a comma seperated list. As attempting to apply multiple labels through
  repeated upload of the same package will fail as the package already
  exists, bump the version number of the test package to generate
  multiple packages that can be uploaded with different labels.
Copy link
Member Author

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

If we wanted to also check what labels are uploaded as a validation step in cleanup before removing them we could add something like

curl --silent https://api.anaconda.org/package/scientific-python-nightly-wheels/test-package | \
  jq -r '.releases[].version' > package-versions.txt
for package_version in $(cat package-versions.txt); do
  echo "# scientific-python-nightly-wheels/test-package version ${package_version} labels:"
  curl --silent https://api.anaconda.org/package/scientific-python-nightly-wheels/test-package | \
    jq -r '.files[] | \
    select( .version == "${package_version}" )' | \
    jq -r '.labels'
done

but I don't think that's strictly necessary, so I've left this out.

Comment on lines +57 to +61
run: |
# Bump version to avoid wheel name conflicts
sed -i 's/0.0.1/0.0.2/g' tests/test_package/pyproject.toml
rm ./dist/*
python -m build --outdir ./dist tests/test_package
Copy link
Member Author

Choose a reason for hiding this comment

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

As attempting to apply multiple labels through repeated upload of the same package will fail as the package already exists, bump the version number of the test package to generate multiple packages that can be uploaded with different labels.

@matthewfeickert matthewfeickert marked this pull request as ready for review January 24, 2024 05:42
@matthewfeickert matthewfeickert requested review from bsipocz and a team and removed request for bsipocz January 24, 2024 05:42
@@ -36,7 +36,7 @@ jobs:

- name: Build a wheel and a sdist
run: |
PYTHONWARNINGS=error,default::DeprecationWarning python -m build --outdir ./dist tests/test_package
python -m build --outdir ./dist tests/test_package
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing this as I had added in previously from another workflow I have, but we don't care if there are deprecation warnings in the package build tools, as we just need an artifact to upload.

Copy link
Member

Choose a reason for hiding this comment

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

that assumes that the command will keep working as is, which I think is a valid assumption (or will be a super simple fix many years down the line), so 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the main reason I removed this is that I've had this check in other work flows and this has ended up causing things to fail for reasons unrelated to build itself (e.g. a build backend deprecating something for the future but that warning still getting caught). I agree that if build ends up having issues though we will not be the first to catch it. :)

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

I only have extreme nitpicks, and also wondered about the same thing you wrote whether the labels need to be verified. I don't necessarily think, so would say go ahead and merge this without that check.

@@ -36,7 +36,7 @@ jobs:

- name: Build a wheel and a sdist
run: |
PYTHONWARNINGS=error,default::DeprecationWarning python -m build --outdir ./dist tests/test_package
python -m build --outdir ./dist tests/test_package
Copy link
Member

Choose a reason for hiding this comment

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

that assumes that the command will keep working as is, which I think is a valid assumption (or will be a super simple fix many years down the line), so 👍

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
matthewfeickert and others added 2 commits January 24, 2024 11:58
Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>
Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>
@matthewfeickert matthewfeickert merged commit b579d79 into main Jan 24, 2024
2 checks passed
@matthewfeickert matthewfeickert deleted the ci/test-lables branch January 24, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants