Skip to content

Conversation

@moelasmar
Copy link
Contributor

chore(partial-release): split bump pr into multiple prs

azarboon and others added 18 commits January 24, 2025 09:25
### Issue # (if applicable)

Closes #<issue number here>.

### Reason for this change

Anecdotally, contributors often encounter the "This branch is out-of-date with the base branch" message, which can be confusing. Since I couldn’t find a clear explanation, I sought clarification from one of the admins in [this comment](#32889 (comment)). I’ve summarized their guidance to help other contributors navigate this issue more easily.

### Description of changes

Added clarification on a common "error" in the contributor guidelines.

### Describe any new or updated permissions being added

### Description of how you validated changes

An admin provided guidance on the issue, and it resolved the problem effectively in my case.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

(cherry picked from commit 74bd8ce)
The PR linter code was a bit of a mess; evaluating rules and mutating the PR was interspersed, generic GitHub code was mixed with CDK-specific code, the linter could be triggered from multiple sources, none of them were documented very well.

Try to rectify all of that in this PR to make it easier to extend the PR linter in the future:

- Split the linter into clear evaluate/act responsibilities.
- Split code across more than 1 file.
- Document how the "PR Linter Trigger" works
- Streamline how we get a PR number into the linter.
- Give an example of how to run it locally to test the rule evaluation on real PRs

Not every crazy design decision has been rectified yet, but at least we have a start of something a little more comprehensible. Another change I made: the old PR linter creates a comment + a review with the same content (but not quite). In this PR, make it just do reviews and don't do comments.

This started from a PR that had CodeCov changes added, but I want to do a refactor without feature changes first before adding new code.

----

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

(cherry picked from commit 680b6ba)
Almost every PR immediately looks like it's failing with a red cross, because the PR linter fails if it is requesting changes.

The "Changes Requested" review by itself is enough to prevent a PR from getting merged by the Mergify config, so we don't actually need to fail the PR linter as well.

Instead: the PR linter succeeds if it runs to the end, and it may request changes on the PR. If it fails, then it's because it was unable to do its job for some reason (that should and will still block merging, so we are not accidentally failing open if something is wrong with the linter).

----

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

(cherry picked from commit 2afdf25)
### Issue

Closes #32940

### Description of changes

Define the API for the synth action. Includes DX improvements for some other APIs.

### Describe any new or updated permissions being added

n/a

### Description of how you validated changes

These are the tests!

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

(cherry picked from commit 63b0936)
Since the buffered console captures stdout/stderr, in some call
sequences it keeps recursing forever and overflows memory.

It does not repro in this repository, but it repros in a different one.

The fix is to stop capturing while we print results.

----

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

(cherry picked from commit 776620d)
I suspect that `check_suite` is a useful event to use for the PR linter.

Add a workflow that will trigger on `check_suite` and prints some relevant information, so we can spy on.

This workflow was created by AI, we'll see how it does.

----

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

(cherry picked from commit c9d1684)
In lack of a public docs page, use typedoc for now.

### Description of how you validated changes

Docs only

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

(cherry picked from commit 88fe797)
…dk package (#32989)

Instead use local file references.
We still have it listed as a dev dependency, because we do need the cli build in the monorepo before the toolkit.
Also adds a script to publish a "public" version locally

### Describe any new or updated permissions being added

n/a

### Description of how you validated changes

It builds and the "published" package can be used successfully

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

(cherry picked from commit d16482f)
### Issue #32994

Closes #32994

### Reason for this change

Previously it was not possible to provide external context.

### Description of changes

Cloud Assembly Source Builder now optionally take a Context object that is provided to the source when the assembly is produced.

### Describe any new or updated permissions being added

n/a

### Description of how you validated changes

Unit tests

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

(cherry picked from commit ebe9580)
adds toolkit tests for deploy

----

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

(cherry picked from commit 794520c)
### Issue # (if applicable)

### Reason for this change

Same as this PR #32976

(cherry picked from commit 4285f1e)
Caused much confusion as to whether the docs or the code was wrong. 99% sure its the docs. Will make the same changes in toolkit in a separate PR.

----

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

(cherry picked from commit 4a76fee)
These are tests

----

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

(cherry picked from commit 8c1be1e)
…ions (#32838)

### Issue # (if applicable)

Closes #<issue number here>.

### Reason for this change

When you update multiple aspects of a Lambda function by modifying an
`aws-cdk-lib.aws-lambda` L2 construct and deploying in a single CDK
deployment, you may encounter a short period of time where errors occur
due to all aspects not being updated together.

### Description of changes

Add documentation in `aws-cdk-lib.aws-lambda` to explain this potential
situation.

### Describe any new or updated permissions being added

None

### Description of how you validated changes

None. Only updated README.md

### Checklist
- [X] My code adheres to the [CONTRIBUTING
GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and
[DESIGN
GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Grace Luo <[email protected]>
(cherry picked from commit 6c5accd)
### Description of changes

We currently have to maintain a global singleton `CliIoHost` until we
have passed the ioHost through all the layers for logging. Previously
the global settings for this `IoHost` were all over the place using
setter functions and global variables. This refactor unifies all these
APIs on the `CliIoHost`, through the global instance.

We also need the ability to register a _different_ `IoHost` that must be
used for reporting. This is the case when a Toolkit integrator provides
a custom implemenation.

### Describe any new or updated permissions being added

no

### Description of how you validated changes

Existing and updated test cases.

### Checklist
- [x] My code adheres to the [CONTRIBUTING
GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and
[DESIGN
GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

(cherry picked from commit 72e089b)
…hart to include major and minor contributions (#32619)

### Issue # (if applicable)

Closes #<issue number here>.

### Reason for this change

Make it easier for those new to contributing and for those submitting small contributions to get started.

### Description of changes

Provide introductory information on what a contribution is. Add a new definition of *major* and *minor* contributions. The purpose of these new terms are to distinguish between two different types of contributions for two reasons: (1) Provide a clearer and simpler path to contributing for those submitting small changes like doc improvements or bug fixes; and (2) Provide a separate path of submitting an RFC to discuss implementation details before someone puts in the time and effort of submitting a major contribution.

I also updated the flowchart to highlight these two paths and show how they differ.

Next steps after this revision are to improve the "getting started" documentation and restructure content based on the updated flowchart.

### Describe any new or updated permissions being added

<!— What new or updated IAM permissions are needed to support the changes being introduced ? -->

### Description of how you validated changes

### Checklist
- [ ] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

(cherry picked from commit d8cd4bd)
Reverts #32976

After discussing with team, I'm going to revert the original PR.

This is because we notice that CDK when bundling supports the following feature: https://docs.aws.amazon.com/cdk/api/v2/python/aws_cdk.aws_lambda_nodejs/ICommandHooks.html
```
beforeBundling: Commands in this hook run before the bundling process begins, outside the Docker container. These are executed on the local machine.
beforeInstall: Commands in this hook run inside the Docker container before npm install or npm ci commands are executed.
afterBundling: Commands in this hook run inside the Docker container after the bundling process completes.
```
This means that users can provide custom commands to run inside the docker container and we do not know what current users run. They could provide a command that require root access and this will be a regression once released.

(cherry picked from commit 28067b0)
Add tests for the destroy action

----

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

(cherry picked from commit 1889527)
@moelasmar moelasmar requested a review from a team as a code owner January 24, 2025 17:27
@github-actions github-actions bot added the p2 label Jan 24, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team January 24, 2025 17:27
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 24, 2025
@moelasmar moelasmar added pr/no-squash This PR should be merged instead of squash-merging it and removed contribution/core This is a PR that came from AWS. labels Jan 24, 2025
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 24, 2025
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 fails with the following errors:

❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/33148/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

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.

@aws-cdk-automation
Copy link
Collaborator

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jan 24, 2025
@moelasmar moelasmar added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr/needs-cli-test-run This PR needs CLI tests run against it. labels Jan 24, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review January 24, 2025 17:32

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

@moelasmar moelasmar merged commit 127adc6 into melasmar/v2-release-clone Jan 24, 2025
36 of 42 checks passed
@moelasmar moelasmar deleted the melasmar/v2-release-clone-test branch January 24, 2025 17:34
@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

auto-approve contribution/core This is a PR that came from AWS. p2 pr/no-squash This PR should be merged instead of squash-merging it pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants