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

[FIXED JENKINS-47077] Prevent sympathetic harmonization so that interval actually works #108

Closed
wants to merge 1 commit into from

Conversation

@stephenc
Copy link
Member

stephenc commented Sep 29, 2017

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Sep 29, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

Copy link
Member

jglick left a comment

Not sure I understand the relation in the serial form between PeriodicFolderTrigger.interval and Trigger.spec. Will this mess up saved settings?

CI build seems to be broken, unclear why.

@stephenc

This comment has been minimized.

Copy link
Member Author

stephenc commented Oct 9, 2017

This should only affect the time difference compared not the crontab

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Oct 10, 2017

So you are modifying the return value of toIntervalMillis. That will definitely affect the value of PeriodicFolderTrigger.interval. It could also affect the return value of toCrontab and thus the value of Trigger.spec. Unclear from reading whether the changed value would cause one of the threshold calculations to switch to a new value. You are only subtracting 1s and the comparisons start at 5m, yet doFillIntervalItems specifically offers exactly 5m as an option (for example), meaning

if (millis < TimeUnit2.MINUTES.toMillis(5)) {

could switch from false (an exact match) to true (off by 1000ms). Thus changing the crontab for the 5m option from * * * * * (every minute) to H/12 * * * * (every 5m) IIUC. Intentional?

Saved settings would be affected if you opened and resaved the configuration form.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Oct 11, 2017

BTW please merge with master to recheck the PR build.

@stephenc stephenc force-pushed the stephenc-patch-1 branch Oct 16, 2017
@stephenc stephenc force-pushed the stephenc-patch-1 branch to a684967 Oct 16, 2017
@stephenc

This comment has been minimized.

Copy link
Member Author

stephenc commented Oct 16, 2017

A test would need to run for at least 4 minutes to prove the functionality works... not the kind of test I'd like to add here.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Nov 30, 2017

Understood that testing this would be tricky without some kind of mock clock. Anyway, was

changing the crontab for the 5m option from * * * * * (every minute) to H/12 * * * * (every 5m)

intentional, an accident, or not actually true at all…?

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jan 5, 2018

ping @stephenc

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jan 5, 2018

Related to JENKINS-47077?

@stephenc

This comment has been minimized.

Copy link
Member Author

stephenc commented Jan 8, 2018

@jglick actually I moved the change to only do the delta in the comparison. No change to persistence at all

@stephenc stephenc changed the title Prevent sympathetic harmonization so that interval actually works [FIXED JENKINS-47077] Prevent sympathetic harmonization so that interval actually works Jan 8, 2018
@stephenc

This comment has been minimized.

Copy link
Member Author

stephenc commented Nov 16, 2018

superseded by #119

@stephenc stephenc closed this Nov 16, 2018
@jglick jglick deleted the stephenc-patch-1 branch Nov 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.