-
Notifications
You must be signed in to change notification settings - Fork 68
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
[JENKINS-39968] Atomically create changelog#.xml file during checkout. #31
Conversation
@tmcsantos Thanks very much for the pull request! Unfortunately, when I run it with my test job, I see the same behavior in the return value from the checkout step as I did before the change. Can you confirm that the checkout step is returning the expected value when you use the plugin including the changes in this pull request? If you've confirmed that, could you compare your working case to my non-working case and help me understand my mistake? |
Hi @MarkEWaite In your example you seem to be fetching the same repo, but with different branch/tag. That still fails and is not a concurrent issue, because it fails even in a synchronous way. So in a way, I think that JENKINS-53346 is not directly related to JENKINS-39968. Hope it helped clear some things. |
Thanks for the clarification @tmcsantos . You're absolutely correct! I was surprised at that, since I assumed that the same problem was at the root of reporting changes from
Before this change, they behave the same. After this change, the distinct branches case is much better behaved. This change resolves JENKINS-39968 but does not resolve JENKINS-53346. A fix for JENKINS-53346 will require other changes. |
Thanks @MarkEWaite , as for JENKINS-53346 I may happen to have a fix, I'll submit one in time for the appropriate project. |
@@ -101,7 +101,7 @@ public final void checkout(Run<?,?> run, FilePath workspace, TaskListener listen | |||
if (changelog) { | |||
for (int i = 0; ; i++) { | |||
changelogFile = new File(run.getRootDir(), "changelog" + i + ".xml"); | |||
if (!changelogFile.exists()) { | |||
if (changelogFile.createNewFile()) { |
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.
As I noted in #32 (comment), this seems like it could introduce its own problems.
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 see, I didn't take in consideration failed checkouts, nor that empty changelog files posed a threat. I can go with #32, fixes the same issue, but the proposed change needs some tweaking. see #32 (comment)
see JENKINS-39968 for details.