Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Break generational cycle #2525

Merged
merged 2 commits into from
Oct 21, 2019
Merged

Break generational cycle #2525

merged 2 commits into from
Oct 21, 2019

Conversation

squaremo
Copy link
Member

Adapted from commit msg:

For fluxctl install, we embed a set of templates in the fluxctl binary by generating a fake filesystem as Go code (pkg/install/generated_templates.gogen.go). But the code that generates the fake filesystem also depends on the fake filesystem, since it does double duty as a program for generating example manifests from those templates.

This leads to a problem: if anything has upset the generation process, it's not possible to regenerate the files, since the generation tool won't build.

A related problem, worked around elsewhere (#2473), is that it's easy to introduce spurious differences in the generated code -- which, since it's checked in -- means painful merges.

In general we don't care about the generated Go code, only about what it produces. This PR removes it from git and from the uncommitted change detection (make check-generated); and uses a new capability of fluxctl install to write to individual files to generate the example files under deploy/, removing the need for the code generating program to depend on the code it's generating.

@squaremo squaremo force-pushed the break-generational-cycle branch 2 times, most recently from af7cce7 to c906d37 Compare October 17, 2019 11:55
@squaremo
Copy link
Member Author

NB: there is one change to the example deployments in deploy/ as a result of this: the git label, email and user are set explicitly to example values rather than left to default, since we require them to be supplied to fluxctl install.

I think this is a good thing, but I have little idea of how deploy/ gets used nowadays -- @stefanprodan, do you think it is OK?

@squaremo squaremo requested a review from hiddeco October 17, 2019 12:18
@stefanprodan
Copy link
Member

@squaremo I see no problem with setting git label, email and user, but I would use flux-sync as the label default or better yet use sync-state=secret.

@squaremo
Copy link
Member Author

@stefanprodan Thanks -- I'll change the label. I am reluctant to change too much about either fluxctl install or the example manifests here, so I'll leave the "better yet" to another PR.

Makefile Outdated Show resolved Hide resolved
deploy/flux-deployment.yaml Outdated Show resolved Hide resolved
@hiddeco
Copy link
Member

hiddeco commented Oct 17, 2019

The change is a great improvement for us, both in complexity and code reduction, and improved maintainability.

I am however wondering if there is any impact on eksctl and how they currently make use of the install package, as iirc, they make use of the embedded file that is no longer there.

@stefanprodan
Copy link
Member

stefanprodan commented Oct 17, 2019

I think this breaks eksctl, wksctl, firekube and any other project that depends on the install package.

@squaremo
Copy link
Member Author

squaremo commented Oct 17, 2019

any other project that depends on the install package

WHYYYYYYY
(did they do that)

@squaremo
Copy link
Member Author

any other project that depends on the install package

WHYYYYYYY
(did they do that)

Oh yes, it was the motivation for the install package in the first place.

@2opremio
Copy link
Contributor

2opremio commented Oct 17, 2019

I think it's OK to break the API (eksctl vendors it anyways) but Flux should still supply a way for eksctl to generate the files (ideally in memory and including the filenames)

@hiddeco
Copy link
Member

hiddeco commented Oct 17, 2019

@2opremio would it be possible for them to generate their own embedded file based on the templates they receive from vendoring the package?

@squaremo
Copy link
Member Author

I can commit the generated file, and restore the change detection, while still breaking the dependency that was causing problems. (It's just a shame go doesn't let us avoid it)

@2opremio
Copy link
Contributor

Yes, I guess that would be possible, but in practice that's pushing the problem to the consumer which I don't think it's ideal (particularly if it's not just eksctl which uses this functionality)

@squaremo
Copy link
Member Author

that's pushing the problem to the consumer

Not really -- the consumer wouldn't have the circular dependency problem that's being fixed here.

However, it is fiddly to call go generate on an imported module (in fact, go will refuse to do so). So: I have pinched my nose and reinstated the generated file.

@squaremo squaremo force-pushed the break-generational-cycle branch 2 times, most recently from 02644fe to 69fa669 Compare October 17, 2019 14:10
@squaremo squaremo force-pushed the break-generational-cycle branch from 69fa669 to 658b845 Compare October 21, 2019 10:58
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Thanks for double checking the timestamp issues @squaremo, LGTM 🥇

It's pretty useful to be able to get a flux config as individual
files, so you can e.g., check them into git. This also means we can
use `fluxctl install` to generate the files under deploy/, so they
will be reliably close to what you'd get from running the tool.
For `fluxctl install`, we embed a set of templates in the fluxctl
binary by generating a fake filesystem as Go code
(pkg/install/generated_templates.gogen.go).

But the code that generates the fake filesystem also _depends_ on the
fake filesystem, since it does double duty as a program for generating
example manifests from those templates.

This leads to a problem: if anything has upset the generation process,
it's not possible to regenerate the files, since the generation tool
won't build.

(A related problem, worked around elsewhere (#2473), is that it's easy
to introduce spurious differences in the generated code -- which,
since it's checked in -- means painful merges. The solution there
still works here.)

In general we don't care about the generated Go code, only about what
it produces. This commit removes the generated file from the
"uncommitted change" detection (`make check-generated`); and uses
`fluxctl install` to generate the example files, removing the need for
the code generating program to depend on the code it's generating.

NB It's still necessary to commit the generated code, since other
projects depend on the install package.
@squaremo squaremo force-pushed the break-generational-cycle branch from 658b845 to 3cae80d Compare October 21, 2019 11:03
@squaremo squaremo merged commit c0b0dca into master Oct 21, 2019
@squaremo squaremo deleted the break-generational-cycle branch October 21, 2019 11:40
@2opremio 2opremio added this to the 1.16.0 milestone Nov 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants