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

Remove envsubst dependency #669

Merged
merged 1 commit into from
Sep 11, 2021
Merged

Remove envsubst dependency #669

merged 1 commit into from
Sep 11, 2021

Conversation

bellkev
Copy link
Contributor

@bellkev bellkev commented Sep 9, 2021

1. Issue, if available:

n/a, but discussed with @ellistarn

2. Description of changes:

Moved eksctl config file inline in heredoc instead of in a separate file. The main motivator for this is to remove the dependency on envsubst (which is not installed by default on macOS). Also made a small change to clarify that the cluster creation does not actually create a new IAM role, but rather sets up an IAM OIDC provider to allow subsequent association of IAM roles with Service Accounts.

3. Does this change impact docs?

  • [ x ] Yes, PR includes docs updates
  • Yes, issue opened: link to issue
  • No

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

@geoffcline
Copy link
Contributor

geoffcline commented Sep 9, 2021

I will have to think on this. I had some schemes around automated testing of guides and formatting of yaml I was working on personally. Please don't merge in the next 24 hours.

Edit: also, welcome to the repo, fellow newcomer :)

@geoffcline geoffcline added documentation Improvements or additions to documentation blocked Unable to make progress due to some dependency labels Sep 9, 2021
@netlify
Copy link

netlify bot commented Sep 9, 2021

✔️ Deploy Preview for karpenter-docs-prod ready!

🔨 Explore the source changes: ae1deec

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/613996203ca2c90008fba015

😎 Browse the preview: https://deploy-preview-669--karpenter-docs-prod.netlify.app

curl -fsSL https://karpenter.sh/docs/getting-started/eksctl.yaml \
| envsubst \
| eksctl create cluster -f -
cat <<EOF > cluster.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Iirc it's possible to pass the file directly to eksctl inline to avoid dropping a file on the local filesystem.

Copy link
Contributor Author

@bellkev bellkev Sep 9, 2021

Choose a reason for hiding this comment

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

It is and would work here. Personally I just always want to take a look at the config file before launching, because it takes like 20min to launch cluster + ng and some things are annoying to change later. I wouldn’t object to going back to the eksctl -f - option though. Also, sometimes multi-line pasting can subtly break depending on the terminal emulator, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with the rest of the guide, I'd probably lean doing it all inline.

Personally I just always want to take a look at the config file before launching

I often edit them inline after pasting but before entering them.

Also, sometimes multi-line pasting can subtly break depending on the terminal emulator, etc.

This is a decent point, but at some point, there's only so much we can do to make this smooth with all user environments

Copy link
Contributor

@ellistarn ellistarn left a comment

Choose a reason for hiding this comment

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

Thank you!!

@bellkev
Copy link
Contributor Author

bellkev commented Sep 9, 2021

I will have to think on this. I had some schemes around automated testing of guides and formatting of yaml I was working on personally. Please don't merge in the next 24 hours.

Edit: also, welcome to the repo, fellow newcomer :)

I thought about that and figured it was definitely better not to have a separate copy of the cluster config inline and in another file. I was thinking there might be some kind of “include” tag in hugo that could help, but didn’t know enough about the website build process to attempt it now.

@geoffcline geoffcline removed the blocked Unable to make progress due to some dependency label Sep 9, 2021
@geoffcline
Copy link
Contributor

Started a discussion in slack regarding the merits of inline yaml vs hosting a file. Removing blocked label.

Copy link
Contributor

@ellistarn ellistarn left a comment

Choose a reason for hiding this comment

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

Approved. Happy to reapprove if you accept my last comment.

@ellistarn ellistarn merged commit 2fe306d into aws:main Sep 11, 2021
@ellistarn
Copy link
Contributor

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants