-
Notifications
You must be signed in to change notification settings - Fork 68
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
Refactoring CI workflows #274
Conversation
dc8f043
to
504b420
Compare
|
.github/workflows/comment.yml
Outdated
@@ -1,28 +1,31 @@ | |||
name: CI | |||
name: Comment on PRs with plugin metadata issues |
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.
feel free to delete this file, it's no longer needed (ahmeds PR also deletes it)
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.
I am not so sure. @Ahmed also mentioned preview is not able to be run by pr from forked repos but this one can. So we can keep it for the plugins developers for the moment?
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.
I still think we should delete it, see my comment here #271 (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.
That's fair! I'll remove it.
run: | | ||
mv plugins_metadata.json aiida-registry-app/src/ | ||
cp plugins_metadata.json aiida-registry-app/src/ | ||
cp plugins_metadata.json aiida-registry-app/dist/ |
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.
npm run build
does not already do that?
if not, I think one may need to add the json file in some app manifest (is this correct @AhmedBasem20 ?)
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.
npm run build does not already do that?
Yes it does. I think running mv
command only will do the job. What you think @unkcpz ?
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.
The second copy is for the code at
run: cp ./aiida-registry-app/src/plugins_metadata.json ./aiida-registry-app/dist |
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.
In preview.yml
, the file will not be on gh-pages
without this step. However, it is not needed on webpage.yml
. Not sure why🤷♂️
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.
thanks for making the workflows more modular, much appreciated!
I haven't yet had time to follow the logic - if you would like feedback on something specific let me know
- name: Restore cached Plugins | ||
if: inputs.cache | ||
id: cache-plugins-restore | ||
uses: actions/cache/restore@v3 | ||
with: | ||
path: plugins_metadata.json | ||
key: plugins_metadata |
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.
Thanks @ltalirz. This is not working as expected somehow, for the "preview" action, the cache is set the false and this step still runs and cause generate steps skipped. Do you have any idea why?
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.
Sorry, I don't know the details of how the if conditions work
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.
No worries, I think I find how to exit this rabbit hole. All because of this syntax issue of GitHub action actions/runner#1483 🥲
.github/workflows/comment.yml
Outdated
@@ -1,28 +1,31 @@ | |||
name: CI | |||
name: Comment on PRs with plugin metadata issues |
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.
I am not so sure. @Ahmed also mentioned preview is not able to be run by pr from forked repos but this one can. So we can keep it for the plugins developers for the moment?
11df398
to
7df3c76
Compare
add build/ to gitignore GITHUB_TOKEN not required when running locally try use the new aiida-core image Use reusable actions and refact CI workflow use pyproject.toml cache pip install comment CI only for plugins.yaml action preview based on test-webpage-build
7df3c76
to
4c54fc4
Compare
plugins_metadata.json
is created from scratch for preview and webpage deploymentaiida-registry fetch
locally.