Skip to content
This repository has been archived by the owner on Apr 25, 2024. It is now read-only.

Changed documentation to store cluster name in ENV-variable for more flexibility #549

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

smoell
Copy link
Contributor

@smoell smoell commented Aug 23, 2018

Issue #, if available (include keywords to close issue as applicable, e.g. "fixes <##>"):

Description of changes:

  • Changed documentation
  • Changed shell scripts which get downloaded from S3

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dalbhanj
Copy link
Contributor

Hi @smoell
I'd rather store EKS environment variables used throughout the workshop in bashrc via Build script (lab-ide-build.sh). Do you want to update this build script and resend the PR?

@buzzsurfr
Copy link
Contributor

Let's update the build script to set EKS_CLUSTER_NAME by default, but also provide instructions in the README about how to overwrite.

@smoell
Copy link
Contributor Author

smoell commented Aug 25, 2018

Sounds good, I'll do this. Additional question: why are the shell-scripts pulled from S3 and not part of the repo, I want to change this as well if you agree.

@buzzsurfr
Copy link
Contributor

The scripts are hosted in S3 since the CloudFormation templates also must be in S3. The logic was to have everything you need before cloning the repository in the S3 bucket.

The scripts are kept in the repository, and the goal was to incorporate a CI process that would automatically validate the template/scripts and export to the S3 bucket.

The script also clones the repository, so it’s also a “chicken and egg” scenario.

I would prefer to pull the script from S3, but it won’t make a difference.

@smoell
Copy link
Contributor Author

smoell commented Aug 25, 2018

I see ok, so I'll provide the changed shell scripts directly.

- Changed files to use ENV-variable
- Changed documentation
@dalbhanj
Copy link
Contributor

dalbhanj commented Oct 5, 2018

LGTM. @buzzsurfr if you are good with this PR, let's merge it

Copy link
Contributor

@buzzsurfr buzzsurfr left a comment

Choose a reason for hiding this comment

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

LGTM :shipit: (Sorry for delay, was OOO)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants