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

Fixes #14684 - Save end time for recurring #172

Merged
merged 3 commits into from May 2, 2016

Conversation

adamruzicka
Copy link
Contributor

@adamruzicka adamruzicka commented Apr 19, 2016

@ares
Copy link
Member

ares commented Apr 19, 2016

It does not seem to work for me, when I set end time, the action results in 200 but it redirects me back to the invocation form without any error. Also it creates the job, it creates recurring action but it does not create the task so the job is queued forever. I don't see any error in the log.

@ares
Copy link
Member

ares commented Apr 19, 2016

After short debugging session we realized that if there's no window for job to start, task is not created causing all of this problems. So a validation should be added that checks that $start_time < $end_time

@adamruzicka
Copy link
Contributor Author

[test]

@@ -304,7 +311,8 @@ def trigger!
end

def valid?
targeting.valid? & job_invocation.valid? & !pattern_template_invocations.map(&:valid?).include?(false)
targeting.valid? & job_invocation.valid? & !pattern_template_invocations.map(&:valid?).include?(false) &
Copy link
Member

Choose a reason for hiding this comment

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

since you're already touching this line, would you mind changing

!pattern_template_invocations.map(&:valid?).include?(false)
to
pattern_template_invocations.all?(&:valid?)

@ares
Copy link
Member

ares commented Apr 25, 2016

Tested manually and works fine, except one possible improvement in code this patch touches it's good to merge. Well after we have tests fixed and jenkins will confirm.

@adamruzicka
Copy link
Contributor Author

Wasn't there a specific need for doing it this way? Something like it was needed to force validation of all of the template invocations (which the all? approach wouldn't do)?

@ares
Copy link
Member

ares commented Apr 25, 2016

ah right, .all? is lazy 💡 thanks!

@adamruzicka
Copy link
Contributor Author

Great, glad we didn't miss it

@iNecas
Copy link
Member

iNecas commented Apr 26, 2016

Foreman-tasks 0.7.17 with required change was just released

@adamruzicka
Copy link
Contributor Author

Let's see how the [test] goes

@adamruzicka
Copy link
Contributor Author

[test]

1 similar comment
@adamruzicka
Copy link
Contributor Author

[test]

@ares
Copy link
Member

ares commented May 2, 2016

Works fine, thanks @adamruzicka, merging!

@ares ares merged commit 35a7200 into theforeman:master May 2, 2016
@ares
Copy link
Member

ares commented May 2, 2016

oops I left double comment in commit message, sorry :-)

@adamruzicka adamruzicka deleted the fix/14684-save-end-time branch May 9, 2016 12:45
MariaAga pushed a commit to MariaAga/foreman_remote_execution that referenced this pull request Sep 3, 2021
Fixes #13641 - fix the ordering of the tasks menu items
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants