-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
command: remove SHA256SUM file on plugin removal #12666
Conversation
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.
Nice
shasumFile := fmt.Sprintf("%s_SHA256SUM", installation.BinaryPath) | ||
if err := os.Remove(shasumFile); err != nil { | ||
c.Ui.Error(fmt.Sprintf("failed to remove %s: %s", shasumFile, err)) | ||
c.Ui.Error("You may need to remove it manually") |
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 is a nice touch. My only nit is that it gets printed multiple times if we have multiple failed removal but this is probably not an issue.
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.
Can we remove multiple plugins at one with the command?
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.
If I recall, when you have multiple versions of the same plugin it removes all installed versions. Or am I mistaken?
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 true yep, I forgot about this use case. So yeah indeed, it will print the message as much as it fails to remove, but I would argue this is good? If you have a subset of files that fail to be removed for whatever reason, you should know which ones failed to be removed.
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.
Yeah - it was a nit not a rejection 😉
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.
Does the remove test remove the shasums needed for the other tests?
I feel like we may have some gotcha with the test files that need to be updated or skipped over.
When a user invokes `packer plugins remove', the plugin binary gets removed, but not the corresponding SHA256SUM file. This patch changes this so that when a binary is removed, so is its SHA256SUM file.
387eab6
to
0fd16f7
Compare
From my understanding this is a purpose-made file hierarchy for tests, which we don't keep in-between two tests? We won't impact anything by removing those files I presume. |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
When a user invokes `packer plugins remove', the plugin binary gets removed, but not the corresponding SHA256SUM file.
This patch changes this so that when a binary is removed, so is its SHA256SUM file.