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

run go generate on CI #49

Merged
merged 2 commits into from
Jul 16, 2021
Merged

run go generate on CI #49

merged 2 commits into from
Jul 16, 2021

Conversation

marten-seemann
Copy link
Contributor

Fixes #48.

@marten-seemann
Copy link
Contributor Author

CI fails because #46 is not yet merged.

@mvdan
Copy link
Contributor

mvdan commented Jul 14, 2021

I'd be fully on board with this if code generation was deterministic... but it isn't - if just a few days or a week go by, there's a good chance the generated code will change. Plus, re-generation requires manual review, so I'm not sure that automation avoids human interaction either (see #50).

We could make the generator fully deterministic across time by pinning a hash of the CSV, but then we just move the problem elsewhere - now the manual step is to update the SHA every few weeks or months :)

I think there's a manual step every few months no matter what we do. Periodic checks/reminders could be useful, like a cron job - I'd personally prefer that over making commit CI builds start failing by no fault of their own.

@masih
Copy link
Member

masih commented Jul 14, 2021

I'd be fully on board with this if code generation was deterministic... but it isn't - if just a few days or a week go by, there's a good chance the generated code will change. Plus, re-generation requires manual review, so I'm not sure that automation avoids human interaction either (see #50).

We could make the generator fully deterministic across time by pinning a hash of the CSV, but then we just move the problem elsewhere - now the manual step is to update the SHA every few weeks or months :)

I think there's a manual step every few months no matter what we do. Periodic checks/reminders could be useful, like a cron job - I'd personally prefer that over making commit CI builds start failing by no fault of their own.

I am curious why the code generation is non-deterministic?

@mvdan
Copy link
Contributor

mvdan commented Jul 14, 2021

Because it uses the multicodec CSV as input from its HEAD branch, and that CSV table is updated from time to time.

@mvdan
Copy link
Contributor

mvdan commented Jul 14, 2021

Thinking outloud, in an ideal world this Go code generation would be in the same repo as where the CSV is, then we wouldn't have this complexity with separately-moving HEADs. The output could be a module in a sub-directory in the same root repository, for example.

@masih
Copy link
Member

masih commented Jul 14, 2021

Because it uses the multicodec CSV as input from its HEAD branch, and that CSV table is updated from time to time.

OK so when I hear non-deterministic; I think: Given the same CSV file would the generator generate the same code? And I think this is a "Yes". I'd then argue code generator is deterministic and should be automated.

Want to manually release? fine. But I don't think a human looking at a code generated by running go generate adds much tbh.

@masih
Copy link
Member

masih commented Jul 14, 2021

Thinking outloud, in an ideal world this Go code generation would be in the same repo as where the CSV is, then we wouldn't have this complexity with separately-moving HEADs. The output could be a module in a sub-directory in the same root repository, for example.

A note on HEAD: If a motivation to do things manually is "control" then the generator could be triggered when there is a tag on the repo that contains the CSV. That way we have two manual control points: first is manual tagging of CSV file, and second manual release of code generated automatically.

@marten-seemann
Copy link
Contributor Author

Because it uses the multicodec CSV as input from its HEAD branch, and that CSV table is updated from time to time.

Would it make sense to use a Git submodule here? That way things would be more predictable.

@mvdan
Copy link
Contributor

mvdan commented Jul 14, 2021

OK so when I hear non-deterministic; I think: Given the same CSV file would the generator generate the same code?

At least to me, deterministic means "go generate on the same commit will give the same result when I run it many times, be it today or next week or next year".

Would it make sense to use a Git submodule here? That way things would be more predictable.

I mean, sure, it can't hurt - but it's akin to pinning a commit SHA in the URL like I mentioned before, so you simply move "regularly re-run go generate" to "regularly update the SHA and re-run go generate". Makes it more deterministic for sure, but it doesn't remove the manual review element.

@mvdan
Copy link
Contributor

mvdan commented Jul 14, 2021

Maybe I misunderstood your last comment, actually. If you mean use a submodule to make go generate fully deterministic so CI can enforce it to be up to date, that sounds good to me. Then every few months, or as needed, we can update the submodule to pull in CSV changes and code review how that results in generated code changes.

@marten-seemann
Copy link
Contributor Author

That's what I meant. We won't be able to automatically update the submodule (unless we want to write an Action that does that and then opens a PR, but that's probably overkill), but at least we can enforce that the version of the submodule and the generated code are consistent.

@mvdan
Copy link
Contributor

mvdan commented Jul 14, 2021

Yup that sounds good. Happy to review a PR if you want to take a crack. If we don't fancy submodules, we could always swap the raw github URL to use a commit hash instead of HEAD.

@marten-seemann
Copy link
Contributor Author

If I remember correctly, other repositories also use submodules to include the multicodec repo. That's why we introduced the recursive submodule checkout in the Unified CI Setup, btw.

I added the submodule and changed the code generator to read the file from there.

Copy link
Contributor

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Require a CI job that asserts go generate does not change code in a PR
4 participants