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

Merged information from EKS User Guide #1051

Merged
merged 1 commit into from
Jul 14, 2023
Merged

Merged information from EKS User Guide #1051

merged 1 commit into from
Jul 14, 2023

Conversation

pgasca
Copy link
Contributor

@pgasca pgasca commented Jun 29, 2023

Is this a bug fix or adding new feature?
Documentation update

What is this PR about? / Why do we need it?
Merged missing details from https://docs.aws.amazon.com/eks/latest/userguide/efs-csi.html into the existing sections that were noted as Kubernetes specific.

One reason for this is so that Kubernetes users will have full information needed on GitHub even if they aren't aware of the information in the EKS User Guide. Users will be able to more easily contribute to the content when the EKS User Guide no longer has an equivalent GitHub repo.

There was overlap between both sources, with some extra information in one place or another. After this PR, the EKS User Guide will be updated to point to the GitHub readmes where it makes sense to do so rather than have potentially contradictory information that could go out of sync. This is also more consistent with how more recent storage drivers are handled in the EKS User Guide, such as the Amazon File Cache CSI driver.

What testing is done?
Extra details come from what is currently live for EKS User Guide.

@k8s-ci-robot
Copy link
Contributor

Welcome @pgasca!

It looks like this is your first PR to kubernetes-sigs/aws-efs-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-efs-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 29, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @pgasca. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 29, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 29, 2023
@wongma7
Copy link
Contributor

wongma7 commented Jun 30, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 30, 2023
Ashley-wenyizha
Ashley-wenyizha reviewed Jul 6, 2023
View reviewed changes
docs/README.md Outdated

**To install the driver using images stored in the private Amazon ECR registry**

1. Download the manifest. Replace `release-1.X` with a tag for your desired released version. We recommend using the latest released version. For more information and the changelog on released versions and tags, see [Releases](../releases).
Copy link
Contributor

Choose a reason for hiding this comment

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

Change "1" to a bullet point or other format than numbered here? since there is another "1" below

@Ashley-wenyizha
Copy link
Contributor

Content looks good to me, left comment. Could we also rebase the commits and leave a cleaner commit messages/history?

@pgasca
Copy link
Contributor Author

pgasca commented Jul 11, 2023

@Ashley-wenyizha , the numbering got fixed when I corrected the spacing for the Note inbetween the two steps. I've also added detail about how using the master branch isn't recommended for the driver and gave a link to allow users to see the active branches.

Yes, you can rebase the commits. I tried to figure that out but not sure how to do it from the website itself since I'm just using the GitHub forms to edit the readme files. Could you handle the rebase when merging the PR?

@pgasca
Copy link
Contributor Author

pgasca commented Jul 11, 2023 •
edited
Loading

@Ashley-wenyizha, I added another commit removing mention that master isn't recommended (but keeping the link to active branches). Originally I added these statements based on my understanding from a different storage driver (https://github.com/kubernetes-sigs/aws-fsx-csi-driver/blob/master/docs/install.md), but this same recommendation may not apply to EFS CSI based on mentions of master throughout rest of readme.

Please let me know if anything else is needed for this to be merged.

@pgasca
Combining useful information from EKS User Guide.
9c6415c 
Moved considerations, prereqs, and IAM steps from EKS User Guide. Globally replaced "EFS" and "AWS EFS" with "Amazon EFS".

"Amazon EKS" branding

Globally replaced "EKS" with "Amazon EKS".

Replacement correction.

Global replacement fix.

Updated links to work with move

Merged deploy/install info from EKS User Guide.

Moved IAM policy steps to separate file.

Removed steps that were moved to separate file.

Added new link.

Moved over "Create an Amazon EFS file system" from user guide

Update and rename efs-create-filesystem to efs-create-filesystem.md

Update iam-policy-create.md

Give extra context.

Adding references to other sections, other tweaks

small edits

Update iam-policy-create.md

Update README.md

Merged extra details from EKS User Guide.

Update README.md

Formatting fixes

Update README.md

Cleaned up descriptions of settings.

Update README.md

Update README.md

Merged extra details from EKS User Guide.

Update README.md

Addressed Ashley's comment regarding numbering starting over. Also updated replacing release-X.X with more clear instructions on how to look up latest active branch.

Corrected Branches links.

Removed mention that master isn't recommended.

I originally added these statements based on my understanding from a different storage driver (https://github.com/kubernetes-sigs/aws-fsx-csi-driver/blob/master/docs/install.md), but this same recommendation may not apply to EFS CSI.
@Ashley-wenyizha
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2023
@Ashley-wenyizha
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ashley-wenyizha, pgasca

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2023
@k8s-ci-robot k8s-ci-robot merged commit 9a9ca7c into kubernetes-sigs:master Jul 14, 2023
2 checks passed
@iodeslykos iodeslykos mentioned this pull request Jul 15, 2023
Closed
@iodeslykos iodeslykos mentioned this pull request Jan 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ashley-wenyizha Ashley-wenyizha Ashley-wenyizha left review comments

@nckturner nckturner Awaiting requested review from nckturner

@wongma7 wongma7 Awaiting requested review from wongma7

Assignees

@Ashley-wenyizha Ashley-wenyizha

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pgasca @k8s-ci-robot @wongma7 @Ashley-wenyizha