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: Allow build action to be triggered remotely #47

Merged
merged 8 commits into from
Jun 18, 2024

Conversation

DaniBodor
Copy link
Member

@DaniBodor DaniBodor commented Jun 13, 2024

This change allows the build action to be triggered by an action in a remote repo.
This will be used by eitprocessing as of PR 235, and these PRs should be reviewed together.

EDIT: @wbaccinelli , i had tagged you for review, but have since realized that it wasnt working as intended. I will fix it first and then request a new review when it's working.
--> THIS IS NOW DONE!

Some additional changes were made to the actions. All commits are atomic and can be reviewed independently.
Main additional changes:

  • build actions explicitly use os and python versions from matrix
  • lint action was moved to separate file (it now has very little in common with main build action and otherwise i would need to specify extra conditions to avoid it being triggered remotely)
  • a dedicated action that can be triggered remotely was also added. This can be useful for trying out things in future updates. Not sure it should be kept or deleted.

@DaniBodor DaniBodor requested a review from wbaccinelli June 13, 2024 19:39
@DaniBodor DaniBodor force-pushed the remote_workflow_dbodor branch from 31f6148 to 8b3c77a Compare June 13, 2024 20:35
@DaniBodor DaniBodor removed the request for review from wbaccinelli June 13, 2024 20:35
@DaniBodor DaniBodor force-pushed the remote_workflow_dbodor branch 5 times, most recently from daf501e to 524fc9a Compare June 14, 2024 19:33
@DaniBodor DaniBodor changed the base branch from main to 024_update_tests_wbaccinelli June 14, 2024 19:33
@DaniBodor DaniBodor changed the base branch from 024_update_tests_wbaccinelli to main June 14, 2024 19:34
@DaniBodor DaniBodor force-pushed the remote_workflow_dbodor branch 7 times, most recently from 9b2c7fc to c2c4fbd Compare June 18, 2024 11:42
@DaniBodor DaniBodor force-pushed the remote_workflow_dbodor branch from 3e16f00 to 8407dbc Compare June 18, 2024 12:32
@DaniBodor DaniBodor force-pushed the remote_workflow_dbodor branch from 8407dbc to ece1434 Compare June 18, 2024 12:35
this should do the same as the build action when remotely triggered, but in a separate file for testing purposes
@DaniBodor DaniBodor force-pushed the remote_workflow_dbodor branch from ece1434 to de05e43 Compare June 18, 2024 12:40
@DaniBodor DaniBodor requested a review from wbaccinelli June 18, 2024 12:53
Copy link
Collaborator

@wbaccinelli wbaccinelli left a comment

Choose a reason for hiding this comment

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

Good job! Comments are minor.

@@ -41,28 +41,31 @@ jobs:
run: >
docker run
--rm
-v ${{ github.workspace }}:/ci
-v $GITHUB_WORKSPACE:/ci
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the difference with {{ github.workspace }}?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think nothing (it came up when I was checking things with chatgpt), but found this slightly more readable. Wouldn't have made a separate PR for it, but while I was at it...

Comment on lines +38 to +42
- name: Python info
shell: bash -e {0}
run: |
which python3
python3 --version
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

no... it was already in there. I guess for troubleshooting reasons.

Comment on lines 43 to 50
- name: Upgrade pip and install dependencies
run: |
python3 -m pip install --upgrade pip poetry
poetry install --with test
- name: Check linting and formatting using ruff
run: |
poetry run ruff check
poetry run ruff format --check
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it makes sense to go through poetry for checking the linting, or if we can just pip install ruff and run the check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make the change in the other repo too :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's nice to have a debug workflow ready to be used in case of need

@DaniBodor DaniBodor force-pushed the remote_workflow_dbodor branch 5 times, most recently from bb9dcd9 to 4e07dd5 Compare June 18, 2024 14:55
Lint action will bypass poetry install and always install latest version
of ruff. If the action fails, a message is echoed reminding users to
install the latest version of ruff.
Also, poetry was updated to the newest current version of ruff.
@DaniBodor DaniBodor force-pushed the remote_workflow_dbodor branch from 4e07dd5 to c339a07 Compare June 18, 2024 14:55
@DaniBodor DaniBodor merged commit 92fcf9e into main Jun 18, 2024
2 checks passed
@DaniBodor DaniBodor deleted the remote_workflow_dbodor branch June 18, 2024 15:11
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