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.

Sign up for GitHub

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

Jump to bottom

Only make changes in setup-app and setup-cert if previous config does not already exist #29

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Only make changes in setup-app and setup-cert if previous config does not already exist #29

wants to merge 1 commit into from

Conversation

andrew-nowak
Copy link
Member

What does this change?

As in the title: this means that we can include setup-app in scripts, without devs constantly being prompted to auth for sudo when the config already exists and matches what would have been generated.

How to test

Download, then setup an app.
If you've not already setup the app before, does the config get generated correctly?
Then if you run again, does dev-nginx skip generating the config and restarting the nginx instance?

How can we measure success?

Fewer sudo prompts :)

Have we considered potential risks?

  • There are --force flags if users need config regeneration for some reason.
  • This introduces a dependency on openssl in setup-cert - I think this is sufficiently ubiquitous to be okay, but maybe there are setups where that's not available? Regardless, the script should fall back to current behaviour if it's not available, which I think should be fairly harmless.
  • I'm not at all a ruby developer - please do check my syntax :)

@akash1810
Copy link
Member

akash1810 commented May 5, 2023 •
edited
Loading

Hmm, not sure about this.

We typically execute setup-app or setup-cert within a setup script. Following the model described in https://github.blog/2015-06-30-scripts-to-rule-them-all/, setup is a one time thing, so not unsure that the constant requests for sudo would usually be a thing.

Does your use-case differ from this pattern?

@andrew-nowak
Copy link
Member Author

andrew-nowak commented May 5, 2023 •
edited
Loading

Interesting! I think there's often 3 parts of a local dev environment, some really long-term infra with very infrequent changes (eg. dev-nginx, unequivocally belongs to setup), some short term (eg. sbt run, npm run watch or similar, belongs to start) but also some medium term (eg. application dbs - IME usually put into setup) - I fairly often want to reset the database to the baseline, the easiest way (at least for me!) being to rerun the setup script (and I wouldn't want to move that resetting logic into the start script, because I don't want to reset the db every time I run start), ie. I treat the setup script not just as a first-time load, but as a "reset me to the baseline" script

The example repo linked from that github blog post contains this comment in their setup script, which I think also agrees with my interpretation: https://github.com/github/scripts-to-rule-them-all/blob/2e68071ef33c5c6f0d525db00997cd333ff93e8d/script/setup#L3-L4 , though I appreciate I am possibly an outlier in the guardian in this

@akash1810
Copy link
Member

akash1810 commented May 5, 2023 •
edited
Loading

Aha, yes, I see what you mean.

Looking at the GitHub example, have we merged the role of the bootstrap and setup scripts into setup. That is, is bootstrap for those one-time things like dev-nginx, and setup is for resetting?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andrew-nowak @akash1810