Skip to content
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

Be aware of already existing Ngrok config file #194

Merged
merged 21 commits into from
Mar 23, 2023

Conversation

AndyTitu
Copy link
Contributor

@AndyTitu AndyTitu commented Mar 2, 2023

Based on @arunsathiya 's additions this PR looks deeper into handling an already existing config file.

Thought process

check if --config flag is specified with a path to the already existing config file. If yes, read that file, inject it with sensitive information from 1Password and provision it via in-memory file provisioning. If not, use the default locations for the config file. In this way, configs are also present alongside credentials for each run.

@AndyTitu AndyTitu marked this pull request as draft March 2, 2023 17:27
@AndyTitu AndyTitu self-assigned this Mar 2, 2023
@AndyTitu AndyTitu marked this pull request as ready for review March 2, 2023 17:54
@AndyTitu
Copy link
Contributor Author

AndyTitu commented Mar 2, 2023

@arunsathiya @russorat it would be awesome if you could test these changes against your local ngrok setup as well. You might have some configuration that I missed (i.e. is it okay to use time.Duration for connect_timeout in config, or should I just stick to int?).

@AndyTitu AndyTitu requested a review from accraw March 2, 2023 17:57
@arunsathiya
Copy link
Contributor

This looks like a great next step, @AndyTitu, thank you. 🙌🏽

I haven't taken a proper look at the changes so far, but some quick tests seem to run well for me.

I ran into an error but I am unable to reproduce it at the moment, nor do I remember the exact error message. I'll keep testing for another day or two and followup with my findings.

A minor note in the meantime though: I see several "AuthToken" in the struct definition. Should we update that to "Authtoken", in alignment with the field name?

@AndyTitu
Copy link
Contributor Author

AndyTitu commented Mar 6, 2023

Linter fails unexpectedly: #195. Solving this in #196

SimonBarendse
SimonBarendse previously approved these changes Mar 8, 2023
Co-authored-by: Simon Barendse <SimonBarendse@users.noreply.github.com>
SimonBarendse
SimonBarendse previously approved these changes Mar 14, 2023
@AndyTitu AndyTitu changed the title Be aware of already existing Ngrok config file if --config flag is specified Be aware of already existing Ngrok config file Mar 21, 2023
plugins/ngrok/provisioner.go Show resolved Hide resolved
plugins/ngrok/provisioner.go Show resolved Hide resolved
plugins/ngrok/provisioner.go Show resolved Hide resolved
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.

5 participants