-
Notifications
You must be signed in to change notification settings - Fork 11
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
Clean up the managed directories to avoid external side effect influence #15
Conversation
action.yml
Outdated
run: >- | ||
set -x | ||
; | ||
mkdir -pv | ||
"${{ steps.collection-metadata.outputs.collection-namespace-path }}" | ||
; | ||
mv -v | ||
rsync -a |
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.
We shouldn't support working in a dirty working dir. I was about to suggest doing the cleanup in the post
stage (https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#post) but it looks like only JS actions support that: actions/runner#1478.
So for now, I'd go for rm -rf .tmp-ansible-collection-checkout/
as the first step.
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.
We would need to rm -rf "${{ steps.collection-metadata.outputs.collection-namespace-path }}"
as the destination directory of the move is the issue. I can update this PR if that is the route you prefer.
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.
@webknjaz change pushed
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'd prefer to clean up the whole directory we manage to prevent any similar problems with other dirs. Especially as we never expect external influence on that dir.
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.
As a side note: maybe I'll consider using ln -sf
instead of mv
in the future. But this is out of the scope here.
action.yml
Outdated
run: >- | ||
set -x | ||
; | ||
rm -rf | ||
"${{ steps.collection-metadata.outputs.checkout-path }}" |
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.
This should:
- Clean up the whole directory structure we manage to prevent any further side effects.
- Do (1) as the very first step in this action, before the checkout.
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.
Moreover, I'd probably clean up both dirs, I think. Maybe the whole collections dir too.
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 makes sense, I've implemented the change per your suggestion.
mv
with rsync
as another follow-up here, without removing the directories even a successful job will break subsequent jobs. First job log output
Second job log output
|
To avoid issue #14, I'm proposing using rsync to copy the new collection content and then deleting the original temp directory instead of using mv. I also found the title of this block of code did not represent the action.