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

fix(cli): asset publishing broken cross account #18007

Merged
merged 2 commits into from
Dec 14, 2021

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Dec 14, 2021

In #17668, cross-account S3 asset publishing was broken.

The reason is that the account() function was always broken, using the
default account instead of the target account. However, previously this
function was only called in an irrecoverable situation anyway, and its
failure would be rare.

The recent change also calls this function for logging purposes in
a happy-case scenario, but then triggers an error during the logging.

Fix the invocation to use the right account.

Fixes #17988.


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

In #17668, cross-account S3 asset publishing was broken.

The reason is that the `account()` function was always broken, using the
default account instead of the target account. However, previously this
function was only called in an irrecoverable situation anyway, and its
failure would be rare.

The recent change also calls this function for logging purposes in
a happy-case scenario, but then triggers an error during the logging.

Fix the invocation to use the right account.

Fixes #17988.
@rix0rrr rix0rrr requested a review from a team December 14, 2021 11:21
@rix0rrr rix0rrr self-assigned this Dec 14, 2021
@gitpod-io
Copy link

gitpod-io bot commented Dec 14, 2021

@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Dec 14, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Dec 14, 2021
@@ -50,7 +50,15 @@ class PublishingAws implements cdk_assets.IAws {
}

public async discoverCurrentAccount(): Promise<cdk_assets.Account> {
return (await this.sdk({})).currentAccount();
const account = await this.aws.defaultAccount();
return account ?? {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we return something here if we don't find a default account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:( because the contract of the function says it must return something. Not sure how much of an impact this is going to have... fairly sure this won't have any impact but I was trying to be safe and returning a dummy value and seeing what happens in real life.

@mergify
Copy link
Contributor

mergify bot commented Dec 14, 2021

Thank you for contributing! Your pull request will be updated from master 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: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 8fc1607
  • 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 2fc6895 into master Dec 14, 2021
@mergify mergify bot deleted the huijbers/asset-publishing-broken branch December 14, 2021 13:44
@mergify
Copy link
Contributor

mergify bot commented Dec 14, 2021

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

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
In aws#17668, cross-account S3 asset publishing was broken.

The reason is that the `account()` function was always broken, using the
default account instead of the target account. However, previously this
function was only called in an irrecoverable situation anyway, and its
failure would be rare.

The recent change also calls this function for logging purposes in
a happy-case scenario, but then triggers an error during the logging.

Fix the invocation to use the right account.

Fixes aws#17988.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. package/tools Related to AWS CDK Tools or CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(cli): Cross-account asset publishing broken in v1.135
3 participants