Skip to content

Conversation

@vicheey
Copy link
Contributor

@vicheey vicheey commented Nov 1, 2024

Problem

When customer choose use flag information from samconfig.toml for sync and deploy operations, we assume that all the needed parameters will be available in the samconfig.toml. Only --config-file <samconfig file path> is passed to SAM CLI.

We skip many prompters in such scenario reduce clicking. Yet, we also need to skip prompter for ecrRepoUri and syncFlag in sync wizard.

Solution

Add condition to skip these prompter when flag information from samconfig.toml is used.


License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

github-actions bot commented Nov 1, 2024

  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

@vicheey vicheey marked this pull request as ready for review November 1, 2024 18:59
@vicheey vicheey requested a review from a team as a code owner November 1, 2024 18:59
placeholder: 'Press enter to proceed with highlighted option',
buttons: createCommonButtons(samSyncParamUrl),
})
this.form.syncFlags.bindPrompter(
Copy link
Member

Choose a reason for hiding this comment

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

What would be syncFlags if we skipped here? Will it be empty?

Copy link
Contributor Author

@vicheey vicheey Nov 1, 2024

Choose a reason for hiding this comment

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

I believe that what we discussed last time.
When the customer use samconfig file, we use all the flag values from the file; this mean, we would use --config-file as mention here. The based on the value, the boundArgs is loaded here. We do not need the sync flags for merging with boundArgs here because again, --config-file is used.

Please let me know if there is issue.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is, are the sync flags actually empty if they are skipped. Could you help to confirm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, confirm that it is an empty array

Copy link
Member

@roger-zhangg roger-zhangg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for improving the wizard experience

@roger-zhangg
Copy link
Member

One thing we might want to checkout: if appbuilder failed to build/deploy/sync with the current param. Should we still allow use current param in the next try?

@hayemaxi
Copy link
Contributor

hayemaxi commented Nov 1, 2024

/runIntegrationTests

@vicheey
Copy link
Contributor Author

vicheey commented Nov 1, 2024

The integration tests failures do not seem to be related to the new change introduced in this PR but more like false negative resulted from issue with mocking.

@justinmk3
Copy link
Contributor

The integration tests failures do not seem to be related to the new change introduced in this PR but more like false negative resulted from issue with mocking.

Do you plan to address that here or in a followup pr? It is unusual for integ tests to fail.

  1) SAM Integration Tests
       SAM Sync
         should instantiate ChildProcess with the correct arguments for sync:

@justinmk3 justinmk3 merged commit 65d0e93 into aws:master Nov 3, 2024
27 of 31 checks passed
@vicheey vicheey deleted the fix-sam-deploy-ecr-prompter branch November 4, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants