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

feat(ec2): add support for environment files and variables in systemd #29629

Merged
merged 11 commits into from
May 1, 2024

Conversation

razin99
Copy link
Contributor

@razin99 razin99 commented Mar 27, 2024

Reason for this change

Allow users to define Environment and EnvironmentFile in a systemd service file.

Description of changes

Added 2 new properties for ec2.InitService.systemdConfigFile:

  1. environmentVariables which is a string key value pair
  2. environmentFiles which is a list of file paths

Description of how you validated changes

I have added a unit test.
Documentations referred to:

Checklist


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

@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Mar 27, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team March 27, 2024 12:34
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@razin99
Copy link
Contributor Author

razin99 commented Mar 27, 2024

Exemption Request

No README updates needed since this added optional features that goes beyond basic systemd service creation

@aws-cdk-automation aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Mar 27, 2024
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks for contributing 👍
Left some comments for minor adjustments.
Also, you could add some documentation in Configuring Instances using CloudFormation Init (cfn-init).

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Mar 31, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review March 31, 2024 11:29

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

@razin99 razin99 requested a review from lpizzinidev March 31, 2024 11:32
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks 👍 Added a suggestion to change the wording for the documentation

packages/aws-cdk-lib/aws-ec2/README.md Outdated Show resolved Hide resolved
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 1, 2024
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
@razin99
Copy link
Contributor Author

razin99 commented Apr 2, 2024

@lpizzinidev what do I do from here? Never got a PR this far in this repo. Do I just wait for it to be merged?

@lpizzinidev
Copy link
Contributor

Hey @razin99
First, nice job if this is one of your first PRs!
Now, you need to wait for another round of reviews from one of the core team members.
After the subsequent changes are approved, your PR will be merged 👍

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra 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 for your work on this. I'm thinking that it's likely that this change is good to go with one clarifying question: can a user provide both environmentVariables and environmentFiles or is it an either/or situation?

If they can provide both, this looks ready.

@razin99
Copy link
Contributor Author

razin99 commented Apr 20, 2024

Thank you for your work on this. I'm thinking that it's likely that this change is good to go with one clarifying question: can a user provide both environmentVariables and environmentFiles or is it an either/or situation?

If they can provide both, this looks ready.

They can provide both, as this is a supported behaviour by systemd. From what I've read (here), anything EnvironmentFile can override Environment

Quote from documentation:

Settings from these files override settings made with Environment=. If the same variable is set twice from these files, the files will be read in the order they are specified and the later setting will override the earlier setting.

GavinZZ
GavinZZ previously approved these changes May 1, 2024
Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

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

Thanks for answering the questions and making contribution to CDK repo. Approving on behalf for Kendra since her question is answered and this PR looks good to me.

Copy link
Contributor

mergify bot commented May 1, 2024

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 aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label May 1, 2024
Copy link
Contributor

mergify bot commented May 1, 2024

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).

@mergify mergify bot dismissed GavinZZ’s stale review May 1, 2024 21:36

Pull request has been modified.

Copy link
Contributor

mergify bot commented May 1, 2024

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 90f1aa9 into aws:main May 1, 2024
9 checks passed
Copy link
Contributor

mergify bot commented May 1, 2024

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
Copy link
Collaborator

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants