Skip to content

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

Merged
merged 2 commits into from
May 23, 2023

Conversation

code-asher
Copy link
Member

@code-asher code-asher commented May 9, 2023

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...

@code-asher
Copy link
Member Author

@bpmct wondering if you have any insight on the above?

@bpmct
Copy link
Member

bpmct commented May 9, 2023

To me, #2 seems like the best option here.

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.

@code-asher
Copy link
Member Author

Thanks for the input! That makes sense, I think that would allow for the maximum successful connection scenarios.

@code-asher code-asher force-pushed the configure-cli-path branch from 6f354f3 to aec6c8b Compare May 10, 2023 23:09
@code-asher code-asher changed the base branch from main to version-check May 10, 2023 23:09
@code-asher code-asher force-pushed the configure-cli-path branch 2 times, most recently from 0d7fe0b to ff64951 Compare May 10, 2023 23:26
@code-asher
Copy link
Member Author

code-asher commented May 10, 2023

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.

@code-asher
Copy link
Member Author

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.

Base automatically changed from version-check to main May 16, 2023 19:36
@code-asher code-asher force-pushed the configure-cli-path branch 4 times, most recently from 4560aa1 to 3f780d4 Compare May 17, 2023 22:37
@code-asher code-asher requested review from johnstcn and removed request for johnstcn May 17, 2023 22:45
@code-asher code-asher marked this pull request as draft May 17, 2023 22:46
This is so admins can download the CLI to some restricted location (like
ProgramFiles).
@code-asher code-asher force-pushed the configure-cli-path branch from 3f780d4 to 1f26933 Compare May 17, 2023 23:02
@code-asher code-asher marked this pull request as ready for review May 17, 2023 23:21
@code-asher code-asher requested a review from johnstcn May 17, 2023 23:21
Copy link
Member

@johnstcn johnstcn left a 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.

@code-asher code-asher merged commit 1cf9057 into main May 23, 2023
@code-asher code-asher deleted the configure-cli-path branch May 23, 2023 19:09
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.

3 participants