chore: run release workflows when files change

[thompson-tomo]

This ensures that the release workflows run when the definition is updated. To prevent issues the final step is skipped when triggered via a pr.

[thompson-tomo]

Checks should be able to be resolved by bumping the setup-ruby action via the renovate dashboard.

[thompson-tomo]

@dazuma u might be interested in these changes.

Ideally it would be great if rather than skipping the last step we pass in --dry-run true as arguments when triggered by file change in pr which skips making any changes but still verifies everything else.

[dazuma]

@thompson-tomo Can you describe again what you want to accomplish here? I'm kind of confused. (In particular, some of these workflows are definitely not meant to be triggered in the manner that I think is being done here.)

[thompson-tomo]

The problem we currently have is if a change is made to any of these workflows ie ruby version bumped by renovate the change is not verified as part of the pr proposing the change. Hence this can lead to issues and has done so for this project hence why the pipelines are currently failing.

This seeks to run the Workflow when ever the definition is changed but not trigger any lasting changes aka a dry-run.

Note #1954 is showing the issue perfectly, we are changing the Workflow but we don't know if the pipeline completes/passes. With this PR, we would have confidence that toys successfully installs etc.

[dazuma]

I see. And it looks like a typical issue we're trying to avoid is #1947 which updated the Ruby version without the corresponding update to setup-ruby, thus breaking all those workflows.

We might want to discuss this in the SIG if you're going to be there. I want to push back on this because it's very ugly and confusing, and the hacks that try to detect whether the workflow is being run normally or as part of this pseudo-CI process, could break if these workflows get more complicated in the future.

(On a related note, I also want to stop pinning setup-ruby to a SHA, because it invites this kind of breakage. The setup-ruby README explicitly recommends against doing what we're doing.)

As a compromise, if we feel this kind of test is important, I would suggest we create a separate workflow specifically for detecting these kinds of breakages. It can have a normal pull_request trigger, filtered on the relevant workflow configs, and include the same set of installs as the existing workflows, but not attempt to execute anything at the end. Renovate should update it at the same time as the other workflows, so we can use it as a proxy to test that the installs work.

[thompson-tomo]

I want to push back on this because it's very ugly and confusing.

Actually this dual trigger is already used on other workflows.

hacks that try to detect whether the workflow is being run normally or as part of this pseudo-CI process, could break if these workflows get more complicated in the future.

I would propose we instead set a Workflow environment variable to indicate if it is a dry-run or not.

(On a related note, I also want to stop pinning setup-ruby to a SHA, because it invites this kind of breakage. The setup-ruby README explicitly recommends against doing what we're doing.)

This goes against opentelemetry and openssf/cncf as it would affect our score on https://scorecard.dev/viewer/?uri=github.com/open-telemetry/opentelemetry-ruby-contrib. Also the setup-ruby recommendation doesn't recommend against it but informs you of the limitations which we overcome by using renovate to keep things up to date.

I would suggest we create a separate workflow specifically for detecting these kinds of breakages.

That would catch some cases but not all cases hence preference is to keep it as close as possible to its actual usage and why many cli tools add a dry-run option.

[dazuma]

I don't think this does what we want. It seems like we'd want this workflow to run if type=closed (i.e. normal runs) OR path matches this file (CI runs), but it looks like adding the paths in this way simply adds a filter, i.e. the workflow runs if type=closed AND path matches. That would break normal operation, and does not give us the CI runs either (as evidenced by the fact that this workflow isn't included in the checks for this current PR).

[thompson-tomo]

Thanks for spoting that, I have addressed it.

Note path filters are not apply to closed events as there is no paths hence this should always run on closed and filtered otherwise.

[dazuma]

As I understand it, removing types altogether will revert it to the default, which is [opened, synchronize, reopened]. (Ref: https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request). So now I expect this does not run on closed at all.

[thompson-tomo]

Ahh, I thought default was broader hence have explicitly set it.

[dazuma]

I tested this in a test repository, and unfortunately it doesn't look like it works. The paths filter does apply to the closed type: if a pull request is merged or closed that doesn't touch the given path, the workflow is not triggered.

[thompson-tomo]

Interesting as a number of searches says that it would've so thank you for testing that out. I have made further changes which should hopefully work. 🤞

[dazuma]

I want to push back on this because it's very ugly and confusing.

Actually this dual trigger is already used on other workflows.

I see that. I don't like it there either, but I guess it's just my opinion.

hacks that try to detect whether the workflow is being run normally or as part of this pseudo-CI process, could break if these workflows get more complicated in the future.

I would propose we instead set a Workflow environment variable to indicate if it is a dry-run or not.

Are you suggesting setting an env variable that the final CLI reads and then goes into a "dry run" mode? We could do that, certainly. But see discussion at the bottom of this comment.

I'm more concerned about the process of correctly expanding the workflow trigger to include the CI case, and the process of correctly distinguishing CI from normal invocation cases (i.e. how do we tell whether or not to set the env variable?) These seem messy and precarious. For example, this PR, I believe, already breaks one of the workflows as it stands.

(On a related note, I also want to stop pinning setup-ruby to a SHA, because it invites this kind of breakage. The setup-ruby README explicitly recommends against doing what we're doing.)

This goes against opentelemetry and openssf/cncf as it would affect our score on https://scorecard.dev/viewer/?uri=github.com/open-telemetry/opentelemetry-ruby-contrib. Also the setup-ruby recommendation doesn't recommend against it but informs you of the limitations which we overcome by using renovate to keep things up to date.

I see. That sounds reasonable. (Although it seems that depending on renovate to update setup-ruby in time has already failed once in the case of #1947, so I'm not sure I trust it. That said, I recognize that this PR itself is intended to detect such failures, so that's fine if it's needed to achieve our goal.)

I would suggest we create a separate workflow specifically for detecting these kinds of breakages.

That would catch some cases but not all cases hence preference is to keep it as close as possible to its actual usage and why many cli tools add a dry-run option.

What cases would not be caught that the current PR would catch?

Note: the existing CLI tools do have a dry-run mode, but it's not intended for this case. The current dry-run mode, for example, doesn't actually do a release, but still does all the input checks to make sure the requested release is viable, does all the builds, etc. That is, it's a true dry-run release, and will fail if the release was not specified correctly or would have failed. For a CI invocation, there is no requested release, and it's not clear to me how to reliably construct a fake one that would actually pass dry-run.

If we wanted to do a CI-oriented dry-run, I could augment the CLI tools to take a different argument, or read an environment variable, but then we'd have to decide what such a dry-run should actually test. Maybe it should just verify that the CLI can start up, but not attempt to do anything else.

[dazuma]

Just a reminder here: if #1954 is merged, we should add .toys/release/Gemfile to the pull request paths being watched.

[thompson-tomo]

These seem messy and precarious. For example, this PR, I believe, already breaks one of the workflows as it stands.

I think this breakage has been fixed. In almost all cases the dry-run would be run on pr's.

Although it seems that depending on renovate to update setup-ruby in time has already failed once in the case of #1947, so I'm not sure I trust it

The issue is the setup-ruby action is in the queue to update and with this change the pr to update ruby would've failed until the setup-ruby had been merged.

What cases would not be caught that the current PR would catch?

In current form it catch issues with the gemfile, with the real benefit coming with the dry-run option.

The current dry-run mode, for example, doesn't actually do a release, but still does all the input checks to make sure the requested release is viable, does all the builds, etc.

That sounds exactly like what I would want the dry-run to do. This way when an update to toys occurs via renovate it undergoes all the necessary checks so that we have confidence in merging it.

[dazuma]

Note: I opened #1959, an alternative for discussion purposes. I included what I see as the pros and cons in its description.

[github-actions]

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot