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 documentation for the aws-ecs-1 variant #1053

Merged
merged 2 commits into from
Aug 31, 2020

Conversation

samuelkarp
Copy link
Contributor

Issue number:
#815

Description of changes:

  • New QUICKSTART-ECS.md setup guide for ECS
  • Updates to README.md to document the variant
  • Updates to BUILDING.md to link to the new setup guide

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

Copy link
Contributor

@srgothi92 srgothi92 left a comment

Choose a reason for hiding this comment

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

Additionally, Do you think we should add a line before 138 in your commit and 132 in old Readme.md section Admin Container , conveying how to reach the admin container and SSM session is not the one directly connecting to admin container

QUICKSTART-ECS.md Outdated Show resolved Hide resolved
Copy link
Contributor

@srgothi92 srgothi92 left a comment

Choose a reason for hiding this comment

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

Do you think we should add supported feature list or TODO feature list somewhere ?

QUICKSTART-ECS.md Outdated Show resolved Hide resolved
@samuelkarp
Copy link
Contributor Author

Additionally, Do you think we should add a line before 138 in your commit and 132 in old Readme.md section Admin Container , conveying how to reach the admin container and SSM session is not the one directly connecting to admin container

If you want to propose that as a separate PR, I think that could be reasonable. We do already say at the top of that section: "This container has an SSH server that lets you log in as ec2-user using your EC2-registered SSH key." But maybe we can make that more clear?

(I don't think that should be part of this PR as it's not an ECS-specific change.)

Do you think we should add supported feature list or TODO feature list somewhere ?

I'm planning to update #815 with that information.

@srgothi92
Copy link
Contributor

Do you think we should add supported feature list or TODO feature list somewhere ?

I’m planning to update #815 with that information.

Shouldn’t that be somewhere public as well and linked to QuickStart-ECS doc, for anyone trying out ECS variant knows what features of ECS will work.

QUICKSTART-ECS.md Outdated Show resolved Hide resolved
@samuelkarp
Copy link
Contributor Author

I’m planning to update #815 with that information.

Shouldn’t that be somewhere public as well and linked to QuickStart-ECS doc, for anyone trying out ECS variant knows what features of ECS will work.

Our GitHub issues are public. I don't think it's the right choice to include this information in the quickstart/setup guide document as the primary audience of that is people trying out Bottlerocket for the first time rather than folks who are running their workloads on Bottlerocket.

We don't really have a section in our documentation today where we talk about what kinds of features do and don't work for our Kubernetes variants either. I think #815 is a reasonable place to put the information for now, and we can potentially add a link to issues like @jahkeup suggested here once those are created.

QUICKSTART-ECS.md Outdated Show resolved Hide resolved
BUILDING.md Show resolved Hide resolved
@samuelkarp samuelkarp force-pushed the ecs-documentation branch 2 times, most recently from b07a8ce to 1d32f2a Compare August 23, 2020 21:35
@samuelkarp
Copy link
Contributor Author

samuelkarp force-pushed the samuelkarp:ecs-documentation branch from b07a8ce to 1d32f2a now

This push adds documentation for the new ecs.loglevel setting added in #1062.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
QUICKSTART-ECS.md Outdated Show resolved Hide resolved
QUICKSTART-ECS.md Show resolved Hide resolved
QUICKSTART-ECS.md Show resolved Hide resolved
QUICKSTART-ECS.md Outdated Show resolved Hide resolved
QUICKSTART-ECS.md Outdated Show resolved Hide resolved
@samuelkarp samuelkarp marked this pull request as ready for review August 24, 2020 17:00
BUILDING.md Show resolved Hide resolved
Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

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

This part of QUICKSTART-EKS.md is my fault. It's even less true now than it was at the time:

Finally, you'll need [aws-cli v1](https://aws.amazon.com/cli/) set up to interact with AWS.
(You'll need a [recent v1 release](https://docs.aws.amazon.com/cli/latest/userguide/cli-chap-install.html#install-tool-bundled) with EKS support.)

Please update that to leave out the version number and point to the latest release.

Edit: the links are correct as-is.

README.md Outdated Show resolved Hide resolved
@samuelkarp
Copy link
Contributor Author

Please update that to leave out the version number and point to the latest release.

Looks like @srgothi92 is taking care of that in #1073.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@samuelkarp samuelkarp force-pushed the ecs-documentation branch 2 times, most recently from 145aeb4 to d559daa Compare August 25, 2020 00:45
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

🚴

QUICKSTART-ECS.md Outdated Show resolved Hide resolved
@samuelkarp
Copy link
Contributor Author

⬆️ This push addresses @rothgar's comment about quoting the expression argument to jq.

@tjkirch tjkirch merged commit d781c76 into bottlerocket-os:develop Aug 31, 2020
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.

8 participants