Skip to content

Be aware of already existing Ngrok config file #194

Merged
AndyTitu merged 21 commits into
mainfrom
andi_t/merge-ngrok-config
Mar 23, 2023
Merged

Be aware of already existing Ngrok config file #194
AndyTitu merged 21 commits into
mainfrom
andi_t/merge-ngrok-config

Conversation

@AndyTitu

@AndyTitu AndyTitu commented Mar 2, 2023

Copy link
Copy Markdown
Contributor

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 AndyTitu requested a review from SimonBarendse March 2, 2023 17:54
@AndyTitu

AndyTitu commented Mar 2, 2023

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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

AndyTitu commented Mar 6, 2023

Copy link
Copy Markdown
Contributor Author

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

Comment thread plugins/ngrok/credentials.go Outdated
Comment thread plugins/ngrok/provisioner.go Outdated
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
@SimonBarendse SimonBarendse requested a review from florisvdg March 14, 2023 12:29
Comment thread plugins/ngrok/provisioner.go Outdated
Comment thread plugins/ngrok/provisioner.go Outdated
Comment thread plugins/ngrok/provisioner.go
Comment thread plugins/ngrok/provisioner.go Outdated
@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
Comment thread plugins/ngrok/provisioner.go
Comment thread plugins/ngrok/provisioner.go
Comment thread plugins/ngrok/provisioner.go
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