Skip to content

Conversation

hasbro17
Copy link
Contributor

@hasbro17 hasbro17 commented Sep 9, 2019

Description of the change:
Added version upgrade sections for v0.8.x and v0.9.x.
Of note is the upgrade to v0.8.2 which outlines steps on how to update a project to use Go modules.

Motivation for the change:
Follow up to #1833

@hasbro17 hasbro17 added the kind/documentation Categorizes issue or PR as related to documentation. label Sep 9, 2019
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 9, 2019

To get familiar with Go modules read the [modules wiki][modules-wiki]. In particular the section on [migrating to modules][migrating-to-modules].

- Ensure that you have Go 1.12+ and [Mercurial][mercurial] 3.9+ installed.
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
- Ensure that you have Go 1.12+ and [Mercurial][mercurial] 3.9+ installed.
- Ensure that you have Go 1.12+ (I do not think that The Mercurial is required at all)

Copy link
Contributor

@camilamacedo86 camilamacedo86 Sep 9, 2019

Choose a reason for hiding this comment

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

Also, is required to:

  • create the group.go
  • Ensure in the api files ( for the open API )

For the Object Struct

// +k8s:openapi-gen=true
// +kubebuilder:subresource:status// 

For the Object Status and Spec

// +k8s:openapi-gen=true
  • Re-run the gen-code commands as well.
	operator-sdk generate k8s
	operator-sdk generate openapi

At least I need to check it too in the projects, so I'd recommend checking these points as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding group.go isn't really necessary since it's not a breaking change. It only suppresses a warning from gengo when running the gen-code commands.

But you're right on the second point: I should highlight the need for +kubebuilder tags now that CRD manifests are overwritten in v0.8.x and will remove any manual edits made by users to the CRD.
From the CHANGELOG:

- Go operator CRDs are overwritten when being regenerated by operator-sdk generate openapi. 
Users can now rely on +kubebuilder annotations in their API code, which provide access to most OpenAPIv3
 validation properties (the full set will be supported in the near future, see this PR) and other CRD fields. (#1278)

This probably should have been marked as breaking change to begin with. Well spotted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I can't recall if I had to setup mercurial when I used Go modules but it's definitely part of our pre-reqs:
https://github.com/operator-framework/operator-sdk#prerequisites

Although it might be one of git or mercurial:
https://github.com/golang/go/wiki/GoGetTools

I'll need to double check and remove it from the SDK's pre-reqs if that's not the case.

Copy link
Member

@joelanford joelanford Sep 19, 2019

Choose a reason for hiding this comment

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

I'm fairly sure mercurial was/is required. I'm pretty sure some transitive dependency somewhere comes from a mercurial repo.

Also, just be careful about which kubebuilder tags are mentioned about being needed/supported. We won't switch to controller-tools 0.2.x until the next release, so tags like +kubebuilder:subresource:status that were added in controller-tools 0.2.0 should not be mentioned.

EDIT: Nevermind - looks like you have that covered in line 229 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I've linked to the legacy kubebuilder docs to show that not all marker comments are supported for the controller-tools version tied SDK release v0.8.2.

But // +kubebuilder:subresource:status specifically is supported pre controller-tools v0.2.0 so I've kept that in the example.

Copy link
Contributor

@camilamacedo86 camilamacedo86 Sep 20, 2019

Choose a reason for hiding this comment

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

HI @joelanford, @hasbro17

I just would like to share that I have NO mercurial installed locally and all are working. Maybe it is specific to some kind of operator so. Also, I could not find anything in the code over it. If it is there let's keep but if possible, I'd like to understand to learn where it has been used.

$ mercurial
-bash: mercurial: command not found

Copy link
Member

Choose a reason for hiding this comment

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

@camilamacedo86 The mercurial command is hg, so check if you have that.

Here's the original issue: #1681

Looks like it affected 0.9 at least, not sure about 0.8 or 0.10.

Copy link
Contributor

Choose a reason for hiding this comment

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

I lot of shame now :-P Yes, I have it! All explained tks for the patient with me.

@camilamacedo86
Copy link
Contributor

Hi @hasbro17,

Excellent contribution. I just added small comments based on the tests/and migrations that I did so far. Please, feel free to reach me out if you wish.

k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20190221213512-86fb29eff628
k8s.io/client-go => k8s.io/client-go v0.0.0-20190228174230-b40b2a5939e4
)
replace (
Copy link
Contributor

@camilamacedo86 camilamacedo86 Sep 16, 2019

Choose a reason for hiding this comment

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

May it needs to be updated as #1899

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My first thought is probably not.
There are other ways to solve this problem, e.g using the proxy cache by setting GOPROXY=https://proxy.golang.org.

I don't want to add it to the guide since this isn't an issue specifically with the SDK and something that's easily if you run into the issue.

Although it looks like https://status.apache.org/ is down quite often so maybe if we merge #1899 I can do a follow up.

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Just one nit/typo. Otherwise LGTM

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Sep 19, 2019
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@hasbro17
Copy link
Contributor Author

/test e2e-aws-ansible

1 similar comment
@hasbro17
Copy link
Contributor Author

/test e2e-aws-ansible

replace github.com/operator-framework/operator-sdk => github.com/operator-framework/operator-sdk v0.8.2
```
- Run `go mod tidy` to clean up the `go.mod` file.
Copy link
Contributor

@camilamacedo86 camilamacedo86 Sep 20, 2019

Choose a reason for hiding this comment

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

WDYT about we tell to the user's use the make commands in order to add use the GOPROXY? See

export GOPROXY?=https://proxy.golang.org/

Suggested change
- Run `go mod tidy` to clean up the `go.mod` file.
- Run `make tidy` to clean up the `go.mod` file.

Copy link
Member

Choose a reason for hiding this comment

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

Users don't have a Makefile in their projects, so make tidy wouldn't apply to them. That's just a convenience make target we have for the SDK repo.

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.

Great work 🥇
Just a small nit as suggestion.

LGTM for me.

@hasbro17
Copy link
Contributor Author

/test e2e-aws-ansible

2 similar comments
@hasbro17
Copy link
Contributor Author

/test e2e-aws-ansible

@hasbro17
Copy link
Contributor Author

/test e2e-aws-ansible

@hasbro17 hasbro17 force-pushed the version-upgrade-guide branch from 60ac88b to 7c02e36 Compare September 26, 2019 17:43
@hasbro17
Copy link
Contributor Author

/test e2e-aws-helm

2 similar comments
@hasbro17
Copy link
Contributor Author

/test e2e-aws-helm

@hasbro17
Copy link
Contributor Author

/test e2e-aws-helm

@hasbro17 hasbro17 merged commit 5889b50 into operator-framework:master Sep 29, 2019
@hasbro17 hasbro17 deleted the version-upgrade-guide branch September 29, 2019 22:01
@hasbro17 hasbro17 mentioned this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to documentation. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants