feat(cloud9): support setting environment owner #23878

pattasai · 2023-01-27T16:12:03Z

Closes #22474

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

Setting environment owner .
The 'owner' now could be an IAMuser or Account root user(It allows AWS to determine who has permissions to manage the environment, either an IAM user or the account root user)

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?
- Unit test for ownerarn

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

gitpod-io · 2023-01-27T16:12:13Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

rix0rrr

Great job on the first PR! 🎉

I've left some comments on the code below. Also, about the PR itself:

Your PR title is very mechanical. We try to write our PR titles so that they make sense when users read them in a release announcement. For example, these messages will show up here: https://github.com/aws/aws-cdk/releases/tag/v2.62.0 . Users won't necessarily care about the specific changes to classes that were made, but care more about the feature that was added. Instead of added owner to Ec2EnvironmentProps, something like support setting environment owner might be more clear.
Also make it clear in your PR body why you did the change, how the change works, and what interesting design decisions you made (and why). This will help a reviewer make sense of the change you made.

Flip through the Contributing Guide one more time to make sure you're familiar with the practices of how we do things.

packages/@aws-cdk/aws-cloud9/lib/environment.ts

packages/@aws-cdk/aws-cloud9/test/cloud9.environment.test.ts

rix0rrr · 2023-01-31T15:17:23Z

packages/@aws-cdk/aws-cloud9/test/cloud9.environment.test.ts


+test('can use CodeCommit repo', () => {


Please describe what the test is trying to assert here

packages/@aws-cdk/aws-cloud9/test/cloud9.environment.test.ts

rix0rrr

Oh and because this is a feature, we generally also ask people to write a small section in the README.md about the use case we unlock, why you want this and a small of example of how you're supposed to use it.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

rix0rrr

Almost there!

I know writing [documentation] will be one of the hardest tasks we have to do, but also one of the most important ones. Don't worry if it's not perfect the first time, this takes a lot of practice.

I added some suggestions of what we might write here. Go through them and rewrite your text after seeing what I'm proposing, or accept what I wrote.

rix0rrr · 2023-02-03T14:36:53Z

packages/@aws-cdk/aws-cloud9/README.md

+### create a new Cloud9 environment with an owner as an Iam User.
+
+```ts
+const user = new iam.User(stack, 'User');
+declare const vpc: ec2.Vpc;
+new cloud9.Ec2Environment(this, 'C9Env', {
+  vpc,
+  imageId: cloud9.ImageId.AMAZON_LINUX_2,
+  owner: cloud9.Owner.User(user)
+});
+```


This example isn't necessary any more, the previous example already covers that.

rix0rrr · 2023-02-03T14:37:38Z

packages/@aws-cdk/aws-cloud9/README.md

+```ts
+new cloud9.Ec2Environment(this, 'C9Env', {
+  // provides root account id.
+  owner: cloud9.Owner.AccountRoot('root account id')


Suggested change

owner: cloud9.Owner.AccountRoot('root account id')

owner: cloud9.Owner.AccountRoot('111111111111')

rix0rrr · 2023-02-03T14:45:22Z

packages/@aws-cdk/aws-cloud9/README.md

@@ -104,3 +104,39 @@ new cloud9.Ec2Environment(this, 'C9Env', {
  imageId: cloud9.ImageId.AMAZON_LINUX_2,
 });
 ```
+
+## Specifying Owners
+`Owner` is a user that owns a Cloud9 environment . `Owner` has their own access permissions, resources. And we can specify an `Owner`in an Ec2 environment which could be of two types, 1. AccountRoot and 2. Iam User. It allows AWS to determine who has permissions to manage the environment, either an IAM user or the account root user


Let's try and explain a bit more what this represents.

For example:

Suggested change

`Owner` is a user that owns a Cloud9 environment . `Owner` has their own access permissions, resources. And we can specify an `Owner`in an Ec2 environment which could be of two types, 1. AccountRoot and 2. Iam User. It allows AWS to determine who has permissions to manage the environment, either an IAM user or the account root user

Every Cloud9 Environment has an **owner**. An owner has full control over the environment, and can invite additional members to the environment for collaboration purposes. For more information, see [Working with shared environments in AWS Cloud9](https://docs.aws.amazon.com/cloud9/latest/user-guide/share-environment.html)).

By default, the owner will be the identity that creates the Environment, which is most likely your CloudFormation Execution Role when the Environment is created using CloudFormation. Provider a value for the `owner` property to assign a different owner, either a specific IAM User or the AWS Account Root User.

`Owner` is a user that owns a Cloud9 environment . `Owner` has their own access permissions, resources. And we can specify an `Owner`in an Ec2 environment which could be of two types, 1. AccountRoot and 2. Iam User. It allows AWS to determine who has permissions to manage the environment, either an IAM user or the account root user (but using the account root user is not recommended, see [environment sharing best practices](https://docs.aws.amazon.com/cloud9/latest/user-guide/share-environment.html#share-environment-best-practices)).

rix0rrr · 2023-02-03T14:46:05Z

packages/@aws-cdk/aws-cloud9/README.md

+## Specifying Owners
+`Owner` is a user that owns a Cloud9 environment . `Owner` has their own access permissions, resources. And we can specify an `Owner`in an Ec2 environment which could be of two types, 1. AccountRoot and 2. Iam User. It allows AWS to determine who has permissions to manage the environment, either an IAM user or the account root user
+
+### AccountRoot


Suggested change

### AccountRoot

To specify the AWS Account Root User as the environment owner, use `Owner.accountRoot()`:

rix0rrr · 2023-02-03T14:46:56Z

packages/@aws-cdk/aws-cloud9/README.md

+})
+```
+
+### Iam User


Suggested change

### Iam User

To specify a specific IAM User as the environment owner, use `Owner.user()`. The user should have the `AWSCloud9Administrator` managed policy:

packages/@aws-cdk/aws-cloud9/lib/environment.ts

rix0rrr · 2023-02-03T14:49:40Z

packages/@aws-cdk/aws-cloud9/lib/environment.ts

@@ -136,7 +142,6 @@ export class Ec2Environment extends cdk.Resource implements IEc2Environment {
    }
    return new Import(scope, id);
  }
-


Don't remove this line, we try to keep an empty line between functions so they are easier to visually distinguish.

rix0rrr · 2023-02-03T14:50:50Z

packages/@aws-cdk/aws-cloud9/lib/environment.ts

+ * The class for different types of owners
+ *
+ *


Suggested change

* The class for different types of owners

*

*

* An environment owner

rix0rrr · 2023-02-03T14:51:26Z

packages/@aws-cdk/aws-cloud9/lib/environment.ts

+ */
+export class Owner {
+  /**
+   * import from Owner Iuser


Suggested change

* import from Owner Iuser

* Make an IAM user the environment owner

rix0rrr · 2023-02-03T14:51:55Z

packages/@aws-cdk/aws-cloud9/lib/environment.ts

+
+
+  /**
+   * import from Owner account root


Suggested change

* import from Owner account root

* Make the Account Root User the environment owner (not recommended)

rix0rrr

Looks good!

I'm giving you a provisional approval. That means that it's approved if you do a couple small things, but I trust you to do what I'm asking (or not do it if you feel you have good reasons not to), and I don't necessarily need to see it again afterwards.

How that works is I'm approving it , but I'm putting the pr/do-not-merge label on it so it doesn't get automatically merged yet.

After you've made the final changes, you can remove the do-not-merge label and automation will take over.

If all is well (if you are in the "owners" file in the repository), my approval will not be undone if you change the PR. If my approval gets dismissed when you change the PR, then let me know and I'll reapprove. Also, in that case make a new PR to add your name to the .mergify.yml file in the top level directory, so that doesn't happen again in the future.

rix0rrr · 2023-02-08T09:02:48Z

packages/@aws-cdk/aws-cloud9/README.md

+
+`Owner` is a user that owns a Cloud9 environment . `Owner` has their own access permissions, resources. And we can specify an `Owner`in an Ec2 environment which could be of two types, 1. AccountRoot and 2. Iam User. It allows AWS to determine who has permissions to manage the environment, either an IAM user or the account root user (but using the account root user is not recommended, see [environment sharing best practices](https://docs.aws.amazon.com/cloud9/latest/user-guide/share-environment.html#share-environment-best-practices)).
+
+### To specify the AWS Account Root User as the environment owner, use `Owner.accountRoot()`


I don't think this needs to be a heading.

Suggested change

### To specify the AWS Account Root User as the environment owner, use `Owner.accountRoot()`

To specify the AWS Account Root User as the environment owner, use `Owner.accountRoot()`

rix0rrr · 2023-02-08T09:03:01Z

packages/@aws-cdk/aws-cloud9/README.md

+})
+```
+
+### To specify a specific IAM User as the environment owner, use `Owner.user()`. The user should have the `AWSCloud9Administrator` managed policy


Same here:

Suggested change

### To specify a specific IAM User as the environment owner, use `Owner.user()`. The user should have the `AWSCloud9Administrator` managed policy

To specify a specific IAM User as the environment owner, use `Owner.user()`. The user should have the `AWSCloud9Administrator` managed policy

mergify · 2023-02-08T16:16:46Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-02-08T17:00:10Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: a4c5e8e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-02-08T17:00:33Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

added property and unit test

2a47f13

github-actions bot added the bug This issue is a bug. label Jan 27, 2023

aws-cdk-automation requested a review from a team January 27, 2023 16:12

github-actions bot added effort/small Small work item – less than a day of effort p2 labels Jan 27, 2023

aws-cdk-automation previously requested changes Jan 27, 2023

View reviewed changes

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 27, 2023

owner_props

43ded09

rix0rrr requested changes Jan 31, 2023

View reviewed changes

rix0rrr added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes and removed pr-linter/exempt-readme The PR linter will not require README changes labels Jan 31, 2023

rix0rrr requested changes Jan 31, 2023

View reviewed changes

pattasai changed the title ~~feat(cloud9): added owner to Ec2EnvironmentProps~~ feat(cloud9): support setting environment owner Jan 31, 2023

support setting environment owner

85e85a7

rix0rrr added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/exempt-test The PR linter will not require test changes labels Feb 3, 2023

support setting environment owner

c52b45e

rix0rrr requested changes Feb 3, 2023

View reviewed changes

pattasai added 2 commits February 3, 2023 10:23

support setting environment owner

a695cd5

fix readme error

beaf73c

rix0rrr approved these changes Feb 8, 2023

View reviewed changes

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Feb 8, 2023

fix readme

4f014c1

pattasai removed the pr/do-not-merge This PR should not be merged at this time. label Feb 8, 2023

Merge branch 'main' into pattasai/add-owner-prop

a4c5e8e

mergify bot merged commit 08a2f36 into main Feb 8, 2023

mergify bot deleted the pattasai/add-owner-prop branch February 8, 2023 17:00

pattasai self-assigned this Mar 21, 2023

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cloud9): support setting environment owner #23878

feat(cloud9): support setting environment owner #23878

pattasai commented Jan 27, 2023 •

edited

Loading

gitpod-io bot commented Jan 27, 2023

aws-cdk-automation left a comment

rix0rrr left a comment

rix0rrr Jan 31, 2023

rix0rrr left a comment

rix0rrr left a comment

rix0rrr Feb 3, 2023

rix0rrr Feb 3, 2023

rix0rrr Feb 3, 2023

rix0rrr Feb 3, 2023

rix0rrr Feb 3, 2023

rix0rrr Feb 3, 2023

rix0rrr Feb 3, 2023

rix0rrr Feb 3, 2023

rix0rrr Feb 3, 2023

rix0rrr left a comment

rix0rrr Feb 8, 2023

rix0rrr Feb 8, 2023

mergify bot commented Feb 8, 2023

aws-cdk-automation commented Feb 8, 2023

mergify bot commented Feb 8, 2023

	owner: cloud9.Owner.AccountRoot('root account id')
	owner: cloud9.Owner.AccountRoot('111111111111')

	### AccountRoot
	To specify the AWS Account Root User as the environment owner, use `Owner.accountRoot()`:

	### Iam User
	To specify a specific IAM User as the environment owner, use `Owner.user()`. The user should have the `AWSCloud9Administrator` managed policy:

	* import from Owner Iuser
	* Make an IAM user the environment owner

	* import from Owner account root
	* Make the Account Root User the environment owner (not recommended)


		`Owner` is a user that owns a Cloud9 environment . `Owner` has their own access permissions, resources. And we can specify an `Owner`in an Ec2 environment which could be of two types, 1. AccountRoot and 2. Iam User. It allows AWS to determine who has permissions to manage the environment, either an IAM user or the account root user (but using the account root user is not recommended, see [environment sharing best practices](https://docs.aws.amazon.com/cloud9/latest/user-guide/share-environment.html#share-environment-best-practices)).

		### To specify the AWS Account Root User as the environment owner, use `Owner.accountRoot()`

	### To specify a specific IAM User as the environment owner, use `Owner.user()`. The user should have the `AWSCloud9Administrator` managed policy
	To specify a specific IAM User as the environment owner, use `Owner.user()`. The user should have the `AWSCloud9Administrator` managed policy

feat(cloud9): support setting environment owner #23878

feat(cloud9): support setting environment owner #23878

Conversation

pattasai commented Jan 27, 2023 • edited Loading

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

gitpod-io bot commented Jan 27, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Feb 8, 2023

aws-cdk-automation commented Feb 8, 2023

AWS CodeBuild CI Report

mergify bot commented Feb 8, 2023

pattasai commented Jan 27, 2023 •

edited

Loading