-
Notifications
You must be signed in to change notification settings - Fork 695
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
Test support for cloud management changes #875
Conversation
So Will look into James' comment about |
It does look like 'Clouds' handles the name change successfully I guess the return url after the save is not being done correctly...somewhere... Update: interesting, did some debugging and the test passed. Might be a race condition somewhere? |
Turns out the issue is here: https://github.com/jenkinsci/jenkins/blob/31721319231f19bead0adcbb005b99c84a6f9171/core/src/main/java/hudson/slaves/Cloud.java#L334 |
48f8198
to
9c41753
Compare
I beieve this handles properly the UI changes, while the 404 is managed by the core pr, I see no reason not to ship this fixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should try to move this forward
@Vlatombe I believe this PR is ready for review now. Would you be able to take a look at it? |
Sorry forgot to remove an unnecessary TODO (related to name change fixes). I changed the test so it didn't rely on name changes and just changed a different property. |
dealing with CI flakes |
@res0nance Could you take a look at this PR? Tests fail here when the jenkins version is upgraded. It also revealed a regression in core (See jenkinsci/jenkins#8310) Will continue to work on getting CI to pass. I believe it is infrastructure right now that is making it fail. |
ci now passes |
Hi @car-roll changes look fine would you like this merged or do you still have work to do in this area? |
Hi @res0nance this PR is done so we can merge it 🚀 many thanks! |
The moving of cloud management to multiple pages (jenkinsci/jenkins#7658) appear to have broken some tests.
Bug also discovered when changing cloud name. See https://issues.jenkins.io/browse/JENKINS-71737
Also upstream of #888
cc @Vlatombe
Testing done
Submitter checklist