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

Add pre-install, post-install hooks to install-ptfe.sh #79

Merged
merged 6 commits into from May 8, 2020

Conversation

aaron-lane
Copy link
Collaborator

Background

Primarily, this branch adds pre-install and post-install script hooks to install-ptfe.sh.

Secondarily, this makes some minor development changes:

  • ignores variable and state files
  • adds SSH agent forwarding
  • pins the Terraform CLI version

Asana task

How Has This Been Tested

I provisioned a cluster using the root module and verified that the default scripts were executed on primaries and secondaries.

Test Configuration

  • Terraform Version: 0.12.24

This PR makes me feel

Windows 95 CD

@aaron-lane aaron-lane requested a review from a team as a code owner May 7, 2020 22:02
Copy link
Contributor

@bnferguson bnferguson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh awesome, thanks for doing this over here! So, in AWS (and maybe Azure too?) there's already a startup_script variable that runs pre-install. It seems that preinstall_script will supersede this functionality so we should remove the startup_script along side this addition.

@aaron-lane
Copy link
Collaborator Author

@bnferguson Nice. I didn't consider defining separate scripts for cloud-init. Seems much cleaner. I'll update the GCP one to do the same rather than the script injection.

This is a cleaner solution than calling the scripts from
install-ptfe.sh.
@aaron-lane aaron-lane requested a review from bnferguson May 8, 2020 18:19
Copy link
Contributor

@rogeruiz rogeruiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 🌈 much cleaner. I dig this.

@rogeruiz rogeruiz dismissed bnferguson’s stale review May 8, 2020 18:27

Changes addressed in 51cd744

variables.tf Outdated
Comment on lines 185 to 189
default = <<-EOD
#!/bin/bash

echo 'A post-install script was not provided.'
EOD
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this isn't an explicit suggestion or a blocking question, but do we as a team have an opinionated stance on heredoc format over using file() for non templated things, this script is small enough to not run into any issues, but to align expectations I'm wondering if we want to put all these in the files/ dir as like postinstall.default.sh

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that sounds reasonable!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does b87a7fc look?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Copy link
Contributor

@erindatkinson erindatkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🌈

@aaron-lane aaron-lane merged commit 4c2b759 into internal-preview May 8, 2020
@aaron-lane aaron-lane deleted the aaron-lane-install-hooks branch May 8, 2020 21:04
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.

None yet

4 participants