-
Notifications
You must be signed in to change notification settings - Fork 7
feat: generate wheels package with underscores instead of dashes #907
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
jorgepiloto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left one suggestion for this, @rborsaru-ansys. Knowing that the build library accepts now a working directory, maybe we can revisit the tests job in this file.
|
Hi @rborsaru-ansys, I don't know why this PR is open but if you want to test a change to any action in this repo, could you simply update your CICD workflow and reference this branch, e.g. ansys/actions@v10 -> ansys/actions@fix/unify-artifacts-file-names, instead of opening a PR ? |
|
Hi @jorgepiloto and @SMoraisAnsys , Thank you. |
jorgepiloto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, @rborsaru-ansys. Thanks for fixing this.
SMoraisAnsys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the contribution @rborsaru-ansys
RobPasMue
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raised my concerns in #681 - but this implementation is great @rborsaru-ansys ! Thanks a lot!
SMoraisAnsys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes to avoid this PR to be merged as long as the discussion in #681 is not over.
jorgepiloto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there, just left one minor comment.
.github/workflows/ci_cd_pr.yml
Outdated
| python-version: ${{ matrix.python }} | ||
| working-directory: .ci/ansys-actions | ||
|
|
||
| package-library: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, we could reuse this in other parts of the code before uploading the dist to the PyPI Test. We can try to do this in a separate pull-request.
|
I have fixed the build-library action code to properly use the working-directory. Now my job only fails because of the final tests and that is because of the name of the library. |
Initial commit for this branch