fix: sam delete not picking up global parameters from samconfig.yaml #6541
Conversation
| if not self.stack_name: | ||
| self.stack_name = config_options.get("stack_name", None) | ||
| # If the stack_name is same as the one present in samconfig file, | ||
| # get the information about parameters if not specified by user. | ||
| if self.stack_name and self.stack_name == config_options.get("stack_name", None): |
There was a problem hiding this comment.
Do we still need to have this logic in the delete code somewhere? If I remember correctly, the config decorator will load all parameters into the click context.
So if the options in the config were all related to stack xyz, and I instead did sam delete --stack-name abc, will it use the parameters related to xyz, for stack abc?
The previous behaviour looks like it won't, and instead will just not set the other options listed below, but feel free to call this out if's a non issue
There was a problem hiding this comment.
This is a good catch. We may run integration tests manually to see if there are some tests which validates this behavior.
It is also weird that we only set these parameters if stack name matches, if not we are just leaving that as it is, which might fail the command there 🤔
There was a problem hiding this comment.
I tried it out with my changes, and the behaviour now is that it will prioritize parameters passed in via the cli options over config values, but theres no logic to skip setting the other variables if the stack name differs between the config and cli parameter
| @@ -149,14 +121,12 @@ def test_delete_no_user_input( | |||
| def test_delete_context_valid_execute_run(self, get_boto_client_provider_mock, patched_click_get_current_context): | |||
| patched_click_get_current_context = MagicMock() | |||
| with DeleteContext( | |||
| stack_name=None, | |||
| stack_name="test", | |||
There was a problem hiding this comment.
Did something else get read in for these values to be changed to test?
There was a problem hiding this comment.
Yea it was reading in some values from a config file somewhere, but since all that happens outside of the context now i had to set the values here
| @@ -73,26 +67,6 @@ def test_delete_context_enter(self, get_boto_client_provider_mock): | |||
| ) | |||
| ), | |||
| ) | |||
| @patch("samcli.commands.delete.delete_context.click.get_current_context") | |||
There was a problem hiding this comment.
Why did we remove this test? This should resolve as it was written since those are the provided values right?
There was a problem hiding this comment.
Since we're using the decorator to read the config and its values all the parsing of the config file happens there instead, and the DeleteContext just has the values passed in now instead of doing any parsing.
Which issue(s) does this change fix?
#6449
Why is this change necessary?
the delete command was doing its own parsing of the config file
How does it address the issue?
switch to use the decorator that reads values from config files and passes those to the function
What side effects does this change have?
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.