-
Notifications
You must be signed in to change notification settings - Fork 16
Allow configuring CLI directory separately from data #246
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
Conversation
@bpmct wondering if you have any insight on the above? |
To me, #2 seems like the best option here.
|
Thanks for the input! That makes sense, I think that would allow for the maximum successful connection scenarios. |
6f354f3
to
aec6c8b
Compare
0d7fe0b
to
ff64951
Compare
All done! One thing that occurred to me about falling back is that it means admins who already have the CLI in the image will still need to set a download source that serves signed binaries otherwise even if the plugin uses the fallback it will not be able to execute the unsigned binary anyway. So (on Windows at least unless we can sign our own binaries) you will always have to configure the download setting in addition to this setting. Still, this will at least avoid an extra download when the correct CLI version is already in the image. We could also take the stance that if the CLI is in the image already then we should always use that no matter what the version is (or make that toggleable) but I am not sure how often backwards compatibility is broken or how often this desync will happen. We could also try something like going back to the old CLI if the newly downloaded CLI fails although I would need to see if we can detect if the failure is due to signing issues. |
Hmm yeah thinking about it more, I will just add the toggle so admins/users can just decide on their own what behavior works best for their setup. |
4560aa1
to
3f780d4
Compare
This is so admins can download the CLI to some restricted location (like ProgramFiles).
3f780d4
to
1f26933
Compare
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.
Really like the table-driven test that runs the entire gamut of possibilities for this! My one issue is that there doesn't seem to be any way to see which CLI version is being chosen -- it would be helpful if we had some form of logging of this process so that we can troubleshoot this later if need be.
This is so admins can pre-seed images with the CLI in some restricted location (like ProgramFiles) if for some reason they are not able to make downloading work (which would be the preferred solution if possible) and they need to put the binary in a restricted location to get around security restraints.
This will break if they forget to add the CLI or add the wrong version or the deployment updates and the user still has the old CLI because the plugin will try to download a new CLI but it will not have permission to write to that restricted location. Maybe there is a better way to handle this...some ideas:- Bypass version checks if the CLI directory is not writable (or make that behavior opt-in with a checkbox). Error if the CLI does not exist.- Fall back to the data directory if the CLI directory is not writable (or make that behavior opt-in with a checkbox) and we are out of date or the CLI does not exist.- Leave it alone and let it error; users will have to change their settings or ping an admin to add/update the CLI.Edit: The fallback behavior can be configured now. So if you place the CLI in a restricted location you can choose whether to force always using that CLI (at the risk of being out of date and an error if the CLI does not actually exist) or allow downloading a fallback to the data directory. The default behavior is to update if needed and fail if it cannot write. I think this should cover all the use cases.
Hopefully I have not just overcomplicated things though...