Skip to content

Doc: Adding documentation for multi-arch operators #5178

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 7, 2021
Merged

Doc: Adding documentation for multi-arch operators #5178

merged 1 commit into from
Oct 7, 2021

Conversation

fgiloux
Copy link
Contributor

@fgiloux fgiloux commented Aug 31, 2021

Description of the change:

This change aims to enhance the documentation by providing guidance on how to build multi-arch operators under advanced topics.

Motivation for the change:

More and more operator authors are looking at getting their operators to support multiple architectures. This intends to provide some help for it.

@openshift-ci openshift-ci bot requested review from fabianvf and theishshah August 31, 2021 09:38
@fgiloux
Copy link
Contributor Author

fgiloux commented Sep 8, 2021

Is there anything missing to get this PR merged?

rashmigottipati
rashmigottipati approved these changes Sep 8, 2021
View reviewed changes
Copy link
Member

@rashmigottipati rashmigottipati left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2021
@rashmigottipati
Copy link
Member

rashmigottipati commented Sep 13, 2021 •
edited
Loading

/hold

@openshift-ci
Copy link

openshift-ci bot commented Sep 14, 2021

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2021
@fgiloux
Copy link
Contributor Author

fgiloux commented Sep 20, 2021

@joelanford anything further required from me to get this doc improvement merged?

@asmacdo
Copy link
Member

asmacdo commented Sep 20, 2021

I don't think these docs apply only to golang-based operators. If the ansible/helm base images support multi-arch, we should move these docs into a more generic location.

@fgiloux
Copy link
Contributor Author

fgiloux commented Sep 21, 2021

I don't think these docs apply only to golang-based operators. If the ansible/helm base images support multi-arch, we should move these docs into a more generic location.

@asmacdo what's your proposal?

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2021
@fgiloux
Copy link
Contributor Author

fgiloux commented Sep 28, 2021

@joelanford @asmacdo is there something additional you wish or can you please merge this PR?

@joelanford
Copy link
Member

/hold cancel

My primary concerns have been addressed. Thanks @fgiloux!

Seems like @asmacdo may still have an open question on the best location for this doc. Most of this doc is generic, but the one piece that's specific to Go is the caveat about setting GOARCH in the Dockerfile.

I also added another comment about that line that I think needs to be addressed before I'm ready to lgtm.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2021
@fgiloux
Copy link
Contributor Author

fgiloux commented Sep 30, 2021

@joelanford
I have fixed GOARCH

@asmacdo I have checked that both:
quay.io/operator-framework/helm-operator:v1.12.0
quay.io/operator-framework/ansible-operator:v1.12.0
are manifest lists. I haven't tested deploying these operators on non amd64 archs. If you think there is a better location for this documentation I am happy to move it. Please let me know.

camilamacedo86
camilamacedo86 reviewed Oct 1, 2021
View reviewed changes
website/content/en/docs/building-operators/golang/advanced-topics.md Outdated
See [docker documentation](https://docs.docker.com/buildx/working-with-buildx/#build-multi-platform-images) for additional options.
**Caveats**: the Dockerfile generated by the SDK for the operator explicitely references GOARCH=amd64 for go build. This can be amended to GOARCH=$TARGETARCH. Docker will automatically set the environment variable to the value specified by --platform. With buildah --build-arg will need to be used for the purpose.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**Caveats**: the Dockerfile generated by the SDK for the operator explicitely references GOARCH=amd64 for go build. This can be amended to GOARCH=$TARGETARCH. Docker will automatically set the environment variable to the value specified by --platform. With buildah --build-arg will need to be used for the purpose.
**NOTE**: the Dockerfile generated by the SDK for the operator explicitly references `GOARCH=amd64` for go build. This can be amended to `GOARCH=$TARGETARCH`. Docker will automatically set the environment variable to the value specified by --platform. With `buildah --build-arg` will need to be used for the purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO this is more a caveat or warning than a note.

camilamacedo86
camilamacedo86 requested changes Oct 2, 2021
View reviewed changes
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

See that not necessary users will do that to integrate with OLM. So, IMHO we need to clarify more it as well. And it seems related to kubernetes-sigs/kubebuilder#2206

@fgiloux
Copy link
Contributor Author

fgiloux commented Oct 4, 2021 •
edited
Loading

See that not necessary users will do that to integrate with OLM. So, IMHO we need to clarify more it as well. And it seems related to kubernetes-sigs/kubebuilder#2206

I am not sure what you are referring to here. In regards to kubebuilder issue 2206 there is a way of building multi-arch images from common sources and it is what I am documenting here. Surely the scaffolded Makefile could allow for that. Therefore buildx and/or buildah targets would need to be added.

@fgiloux
Copy link
Contributor Author

fgiloux commented Oct 4, 2021

@camilamacedo86 I hope the changes meet your expectations. Please let me know.

camilamacedo86
camilamacedo86 approved these changes Oct 4, 2021
View reviewed changes
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Hi @fgiloux,

I am ok with the changes it shows fine. IHMO is only missing to be added outside of the Golang language since it is not specific and only applied to Go projects.

Otherwise, /lgtm

@fgiloux
Copy link
Contributor Author

fgiloux commented Oct 4, 2021

Hi @fgiloux,

I am ok with the changes it shows fine. IHMO is only missing to be added outside of the Golang language since it is not specific and only applied to Go projects.

Otherwise, /lgtm

@asmacdo @joelanford if you agree with @camilamacedo86 's proposal I will move it to a separate section, an option would be: Building Operators > Multi-arch (at the same level as Ansible, Go, Helm).
I am open to other suggestions.

@camilamacedo86
Copy link
Contributor

@fgiloux,

@asmacdo @joelanford if you agree with @camilamacedo86 's proposal I will move it to a separate section, an option would be: Building Operators > Multi-arch (at the same level as Ansible, Go, Helm).
I am open to other suggestions.

Inside of Building Operators we have only the languages.
See the Scorecard and OLM integration sections which are common for all languages.
IMHO that could fit at the same place/level.

Note that @asmacdo also suggested the same already :
#5178 (comment)

@fgiloux
Copy link
Contributor Author

fgiloux commented Oct 4, 2021

@fgiloux,

@asmacdo @joelanford if you agree with @camilamacedo86 's proposal I will move it to a separate section, an option would be: Building Operators > Multi-arch (at the same level as Ansible, Go, Helm).
I am open to other suggestions.

Inside of Building Operators we have only the languages. See the Scorecard and OLM integration sections which are common for all languages. IMHO that could fit at the same place/level.

That would then be a top level section. IMHO it is little content for it. Another option would be to have: "Testing Operators", "OLM Integration" and this new section together under a more generic section. I am fine either way.

Note that @asmacdo also suggested the same already : #5178 (comment)

I know that. I am just asking for an agreement on the new location.

@camilamacedo86
Copy link
Contributor

@fgiloux,

WDYT? Add a new section here such as Advanced Topics and add inside the doc Multiple architectures?

@fgiloux
Copy link
Contributor Author

fgiloux commented Oct 5, 2021

@camilamacedo86 as per your proposal I have created a new section "Advanced Topics" and added the Multiple architectures content there. I let a couple of introductory lines with a link to the new section in the go specific page.
Hopefully this can now get merged

@camilamacedo86
Copy link
Contributor

@fgiloux just a nit and then I can merge this one:
Screen Shot 2021-10-06 at 22 43 21

The order to put overview/install/building operators on top was highly discussed in the past.
So, could we just add the new category to the bottom after OLM Integration? See that they follow a logical sequence.

@fgiloux
Adding documentation for multi-arch operators
c2256e6 
Signed-off-by: Frederic Giloux <fgiloux@redhat.com>
@fgiloux
Copy link
Contributor Author

fgiloux commented Oct 7, 2021 •
edited
Loading

@camilamacedo86 I have moved the menu item "Advanced Topics" below "OLM Integration"

@camilamacedo86 camilamacedo86 merged commit 0ae0cd9 into operator-framework:master Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joelanford joelanford joelanford left review comments

@rashmigottipati rashmigottipati rashmigottipati approved these changes

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@fabianvf fabianvf Awaiting requested review from fabianvf

@theishshah theishshah Awaiting requested review from theishshah

+1 more reviewer

@jmccormick2001 jmccormick2001 jmccormick2001 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rashmigottipati rashmigottipati

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@fgiloux @rashmigottipati @asmacdo @joelanford @camilamacedo86 @jmccormick2001