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 SAP systems policy receiving an empty list of instances #2677

Merged
merged 2 commits into from
Jun 7, 2024

Conversation

dottorblaster
Copy link
Contributor

@dottorblaster dottorblaster commented Jun 7, 2024

Description

Fix SAP systems policy receiving an empty list of instances and marking that as not valid, thus making impossible in some cases to deregister an existing system.

How was this tested?

Existing unit tests passing, added a unit test to make sure this doesn't incur again

@dottorblaster dottorblaster requested a review from CDimonaco June 7, 2024 07:30
@dottorblaster dottorblaster self-assigned this Jun 7, 2024
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Thanks for this @dottorblaster.

The change was introduced here https://github.com/trento-project/web/pull/2214/files, along with other required: true.

I am not sure if any other of those would cause issues.

@nelsonkopliku nelsonkopliku added the env Create an ephimeral environment for the pr branch label Jun 7, 2024
@nelsonkopliku
Copy link
Member

@dottorblaster, if you don't mind, may I just ask to fix this typo making the CI red, please? https://github.com/trento-project/web/actions/runs/9413405002/job/25930084979?pr=2677

@dottorblaster
Copy link
Contributor Author

@nelsonkopliku YES

@dottorblaster dottorblaster force-pushed the fix-empty-instances-sap-system-policy branch from 0046ce9 to 4743f37 Compare June 7, 2024 08:46
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

thanks @dottorblaster, LGTM.

As long as @abravosuse's test is successful and @CDimonaco is fine with this, let's ship it.

@dottorblaster dottorblaster marked this pull request as ready for review June 7, 2024 10:40
@dottorblaster dottorblaster merged commit b5aeb4f into main Jun 7, 2024
26 checks passed
@dottorblaster dottorblaster deleted the fix-empty-instances-sap-system-policy branch June 7, 2024 12:13
@arbulu89
Copy link
Contributor

@dottorblaster Thank you.
This fix means that our e2e tests are "half" complete, because we were testing this feature, but in a different case.
@abravosuse had this issue because when he "deregisters" a SAP instance, he doesn't really clean the env, he just stops the instance, so the profile file is still there.

Our e2e tests check that the cleanup button is there when there is a complete removal of the SAP instance in the agent host, which causes a different payload.

I don't know if we should update the e2e tests, as you added this UT test here, but we have this gap there...

@dottorblaster dottorblaster added the bug Something isn't working label Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working env Create an ephimeral environment for the pr branch
Development

Successfully merging this pull request may close these issues.

3 participants