search

ros-infrastructure / ros_buildfarm

ROS buildfarm based on Docker
Apache License 2.0
81 stars 96 forks source link

Set trigger_timer from build file if unset. #922

Closed nuclearsandwich closed 2 years ago

nuclearsandwich commented 2 years ago

I noticed that when using generate_ci_job.py rather than generate_ci_jobs.py to manually deploy some test jobs that in the former case the scheduled job configuration was not being set.

I'm not sure whether this is an optimal way to resolve this particular issue but I figured that a half-hearted patch was a better starting point than an issue with no code attached.

cottsay commented 2 years ago

Wow, that was certainly an oversight.

Seems that the whole trigger_timer argument is not really needed, and should always be taken directly from the build file. Maybe we should stop passing it explicitly in _configure_ci_jobs and deprecate the argument to configure_ci_job?

nuclearsandwich commented 2 years ago

Seems that the whole trigger_timer argument is not really needed, and should always be taken directly from the build file. Maybe we should stop passing it explicitly in _configure_ci_jobs and deprecate the argument to configure_ci_job?

I think that would still allow the pluralized function to send data pre-loaded to the singular functions since it passes the loaded build file as an argument. I haven't reviewed the devel job behaviors to see if there's a case where you wouldn't want that but I'm in favor knowing what I know.

By what facility would we deprecate the trigger_timer argument, I'm happy to do the work on this PR I just don't know if there's an explicit way to handle deprecating a keyword argument.

cottsay commented 2 years ago

By what facility would we deprecate the trigger_timer argument...

I don't think there's a precedent in ros_buildfarm beyond manually emitting a nastygram to stderr. Maybe it's time we adopt a specific strategy? I'm open to suggestions.

nuclearsandwich commented 2 years ago

9ad721f removes the parameter from the only place where it is set and replacing the conditional that loads from the build file with statements that warn if the argument isn't None and then uses the build_file parameter regardless.

The devel jobs do not use an explicit trigger timer (they're triggered by dedicated trigger jobs and pushes or pull requests) so there is no parallel change to make there. Since the only place where the trigger_timer kwarg was actually used within ros_buildfarm was the configure_ci_jobs function I think there's a case to be made for removing the parameter without a deprecation phase. But I'm not strongly opposed to doing a deprecation first.