Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixes #37938 - Reject start-at dates in the past when a job is scheduled #763

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pafernanr
Copy link

No description provided.

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Code changes look reasonable, apart from that one thing.

image

@MariaAga how much work would it be to mark the date and time fields as incorrect? Also, I vaguely recall the form actively checking that the date is in the future. Has it stopped working in the meantime or am i misremembering?

end
end
future
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be there

@MariaAga
Copy link
Member

@MariaAga how much work would it be to mark the date and time fields as incorrect? Also, I vaguely recall the form actively checking that the date is in the future. Has it stopped working in the meantime or am i misremembering?``

We have it only for starts before, we can probably easliy copy it to starts after as well

@adamruzicka
Copy link
Contributor

@pafernanr could you please add the last newline so that the linter is happy?

@pafernanr
Copy link
Author

I'll review failed CI jobs probably along this week.

@adamruzicka
Copy link
Contributor

@pafernanr ping

@pafernanr
Copy link
Author

Hi, sorry for late response.
I was checking the tests and I think the culprit is https://github.com/theforeman/foreman-tasks/blob/master/test/unit/triggering_test.rb#L12

If I understand it well, that test is exactly executing a job in the past (-120). Dunno why, could be past-times needed for any reason?

@adamruzicka
Copy link
Contributor

If I understand it well, that test is exactly executing a job in the past (-120). Dunno why, could be past-times needed for any reason?

I can't think of anything. The only thing we did there was that jobs scheduled for the past started as soon as possible, making them essentially the same as immediate jobs

@pafernanr pafernanr force-pushed the reject_start-at_in_the_past branch from 95a50e7 to d3d5d2b Compare January 2, 2025 12:35
@pafernanr
Copy link
Author

In that case I removed that test line as it is no longer needed. Executing past dates returns an error which make Unit test to fail.

@pafernanr
Copy link
Author

hmm, I thought it is better idea to keep the test but modifying it in order to check pasta start_at dates returns false. Please check triggering_test.rb. Does it makes sense for you?

Signed-off-by: Pablo Fernández Rodríguez <pafernan@redhat.com>
@pafernanr pafernanr force-pushed the reject_start-at_in_the_past branch from 492ff4f to a2f1ae5 Compare January 2, 2025 16:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants