konveyor/operator

`config/` and `Makefile` bundle variables are out of date

Closed this issue · 15 comments

kaovilai commented

It appears that this repo has been modifying bundle/ without modifying config/

Updates should be put into config/ first, and then run make bundle to generate an update bundle/

For example see here
where we are updating config/rbac/role.yaml

The changes are propagated to bundle/ for you.

image

TL;DR

please do not make changes to bundle/ by hand. Make them in config/ then run make bundle

Tips:

clusterserviceversion in bundle/ generated by make bundle pull values from both config/ and Makefile

For example, bundle channels, operator image tags (ie. podman pull URL) are pulled from Makefile.
https://github.com/konveyor/tackle2-operator/blob/e72591abb03b784d00beeef86c0ef19ff0637e36/Makefile#L22-L42

You will note that utilizing config/ like example above should make editing clusterserviceversion easier for you since you can just edit role.yaml without worrying about its indentations in relations to the final clusterserviceversion yaml.

kaovilai commented

Note that the IMAGE_TAG_BASE is currently incorrect as I doubt konveyor.io is a valid container registry
https://github.com/konveyor/tackle2-operator/blob/e72591abb03b784d00beeef86c0ef19ff0637e36/Makefile#L37-L42

kaovilai commented

IMAGE_TAG_BASE is what should be use for what is currently IMG IIUC

kaovilai commented

It's great to own all the steps of catalog/bundle creation/updates but I personally do not believe it is worth the maintenance required to own so many scripts to update CSV and manage releases, and to replicate operator-sdk run bundle as you're still operating within this OLM ecosystem. But that's just my 2cents :D

jmontleon commented

@kaovilai your last comment confuses me. It sounds like in prior comments you're advocating we make updates to config and run make bundle rather than editing the CSV directly, yet you go on to say, ...I personally do not believe it is worth the maintenance required to own so many scripts to update CSV and manage releases...

Can you clarify what you think is wrong with the process we're using and what you're advocating we do i the future?

kaovilai commented

in addition to editing bundle/CSV via config/

kaovilai commented

See https://github.com/kaovilai/oadp-operator/blob/edge/.github/workflows/edge-catalog.yml

I am able to periodically update an existing catalogsource with a new upgradable version with very minimal scripts to cut releases.

jmontleon commented

Thanks for clarifying. I can't speak to why almost any of those were created.

I take issue with https://github.com/konveyor/tackle2-operator/blob/main/tools/konveyor-operator-publish-commands.sh though. This script is doing a lot more than editing the CSV. It is creating PRs against community-operators and community-operators-prod. It's also replacing tags with SHAs so we work in disconnected. It is possible parts of it could be replaced but I don't think it can be eliminated.

kaovilai commented

Replacing SHAs for disconnected can be done with operator-sdk generate bundle --use-image-digests when config/ is setup properly and the manager.yaml has RELATED_IMAGE_<> setup even with tags, it will replace with shas for you.

kaovilai commented

at least if I understand that correctly.

kaovilai commented 
	if c.useImageDigests {
		c.println("pinning image versions to digests instead of tags")
		if err := c.pinImages(filepath.Join(c.outputDir, "manifests")); err != nil {
			return err
		}
	}

https://github.com/operator-framework/operator-sdk/blob/master/internal/cmd/operator-sdk/generate/bundle/bundle.go#L231-L236

kaovilai commented

The reason why config/ exists is to allow easy maintenance of bundle/. In this case you can leave tags in config/
bundle/ will have pinned SHAs

jmontleon commented

We're not looking to have SHAs in bundle for latest though. It seems like it would be almost impossible to keep it up to date with all the other repos rebuilding on every commit. The desire for SHAs is purely for releases publish to community-operators(-prod).

Having said that it seems like we could run this with konveyor-operator-publish-commands.sh when we want to pin SHAs though.

Do you have a sense of how much updating config/ needs to get back in sync with other changes that have been made? I'm not opposed to this proposal. If we can run a one line operator-sdk command to get the same effect and eliminate 9-10ths of that script it works for me.

Someone just needs to take the time to put config back in sync, figure out what all this other stuff is being used for, make sure we're not breaking anything by removing it, and clean up.

I will say allow easy maintenance of bundle/ seems kind of meh to me in most regards.

As far as I know we're not using scorecard for anything. Effectively leaving four whole files to edit; two we basically never touch. It's not really hard... but I don't disagree there are potential benefits. The script I've pointed to is kind of blinding and I'd be happy to see it mostly go away, for instance.

bundle
├── manifests
│   ├── konveyor-operator.clusterserviceversion.yaml
│   ├── tackle.konveyor.io_addons.yaml
│   └── tackle.konveyor.io_tackles.yaml
├── metadata
│   └── annotations.yaml
└── tests
    └── scorecard
        └── config.yaml
kaovilai commented

We're not looking to have SHAs in bundle for latest though.

--use-image-digests is optional and you can just use it on release branch/tags :)

I would be happy to take this on passively in my spare time. Again no rush.. just pointing out the rainbows.. even if its not much prettier.

kaovilai commented

Do you have a sense of how much updating config/ needs to get back in sync with other changes that have been made?

maybe 10ish files.. you can easily reverse existing bundle/csv into config/