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): Fixing regression for 'container:release' command #3047

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

sbosio
Copy link
Contributor

@sbosio sbosio commented Oct 21, 2024

Description

Here we're fixing a regression introduced when the command was migrated to OClif Core (CLI version 9.x), because of the differences between the old way of specifying a UX action and the new one.

The code section that determines the old release version needs to run before the create release action. It was being run after the creation because the command was written in a way where the actual action (create release request) was assigned to a function, the old version was fetched and then the UX action was called passing the function as the block to be executed. When migrating, the action call was splat in its start and stop calls around the request, but the release version fetching wasn't moved before the action call.

SOC2 Compliance

GUS Work Item

@sbosio sbosio requested a review from a team as a code owner October 21, 2024 19:35
Copy link
Contributor

@eablack eablack left a comment

Choose a reason for hiding this comment

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

LGTM. I want to say that we should add a test here.. but I think the problem is that the patch request impacts the behavior of the get request.. and our mocks are relatively "dumb". So go ahead and ship it unless you can think of a good way to have our tests catch this kind of thing.

@sbosio
Copy link
Contributor Author

sbosio commented Oct 23, 2024

LGTM. I want to say that we should add a test here.. but I think the problem is that the patch request impacts the behavior of the get request.. and our mocks are relatively "dumb". So go ahead and ship it unless you can think of a good way to have our tests catch this kind of thing.

@eablack I didn't added anything because the only way to have avoided this would be specifying a particular order on the Nock interceptors for the tests, but I haven't found any support for that in Nock. Requests matching the same filter will be matched in order (like the GET requests), but there's no support to specify that the first GET request needs to happen before the PATCH request and the following GET request after that one.

Maybe this could be hacked somehow, I thought of using variables to hold the different interceptors and functions on the replies where I could test and throw if the order isn't correct, but it was so messy that I didn't even tried to do it.

@sbosio sbosio merged commit 7fe2ee3 into main Oct 23, 2024
8 checks passed
@sbosio sbosio deleted the sbosio/fix-container-release branch October 23, 2024 15:18
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.

2 participants