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.

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

Add generate Makefile target to generate operator-sdk resources #363

Merged
merged 2 commits into from
Jul 19, 2021
Merged

Add generate Makefile target to generate operator-sdk resources #363

merged 2 commits into from
Jul 19, 2021

Conversation

amuraru
Copy link
Contributor

@amuraru amuraru commented Jul 6, 2021 •
edited
Loading

Change log description

  • Add a new generate Makefile target to generate k8s and CRD resources via operator-sdk
  • Update dependencies:
    • Go 1.13 -> 1.16
    • operator-sdk: 0.17 -> 0.19
    • sigs.k8s.io/controller-runtime v0.5.2 -> v0.6.5
    • k8s v0.17.5 -> v0.19.13

Fixes #364

anishakj
anishakj requested changes Jul 9, 2021
View reviewed changes
Copy link
Contributor

@anishakj anishakj 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.

Can we make 2 different versions of CRD, one for v1beta1 and other for v1. Also upgrade the kubernetes version here so that our e2e tests will pass https://github.com/pravega/zookeeper-operator/blob/master/.github/workflows/ci.yaml#L33

@amuraru
Copy link
Contributor Author

amuraru commented Jul 12, 2021

Can we make 2 different versions of CRD, one for v1beta1 and other for v1.

I couldn't find a way to do that using operator-sdk.
My preference would be to generate v1 raw version and keep the v1beta1 compat in helm chart only

Also upgrade the kubernetes version here so that our e2e tests will pass https://github.com/pravega/zookeeper-operator/blob/master/.github/workflows/ci.yaml#L33

done, thanks for the pointer

@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2021 •
edited
Loading

Codecov Report

Merging #363 (d07e2c2) into master (3b22909) will decrease coverage by 0.25%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #363      +/-   ##
==========================================
- Coverage   86.13%   85.87%   -0.26%     
==========================================
  Files          11       11              
  Lines        1558     1558              
==========================================
- Hits         1342     1338       -4     
- Misses        141      147       +6     
+ Partials       75       73       -2
Impacted Files Coverage Δ
...kg/apis/zookeeper/v1beta1/zz_generated.deepcopy.go 98.03% <ø> (ø)
...er/zookeepercluster/zookeepercluster_controller.go 62.45% <ø> (-0.80%) ⬇️
pkg/zk/synchronizers.go 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b22909...d07e2c2. Read the comment docs.

@anishakj
Copy link
Contributor

anishakj commented Jul 12, 2021 •
edited
Loading

Can we make 2 different versions of CRD, one for v1beta1 and other for v1.

I couldn't find a way to do that using operator-sdk.
My preference would be to generate v1 raw version and keep the v1beta1 compat in helm chart only

Also upgrade the kubernetes version here so that our e2e tests will pass https://github.com/pravega/zookeeper-operator/blob/master/.github/workflows/ci.yaml#L33

done, thanks for the pointer

If we didnt mention crd version here in operator-sdk generate crds v1beta1 crd will get generated

@amuraru
Copy link
Contributor Author

amuraru commented Jul 12, 2021

If we didnt mention crd version here in operator-sdk generate crds v1beta1 crd will get generated

i know that but again do we want to stick with v1beta1 ? It's going EOL in couple of months once k8s 1.22 is released

@amuraru amuraru force-pushed the build branch 11 times, most recently from f236798 to 17024b2 Compare July 12, 2021 12:21
anishakj
anishakj reviewed Jul 12, 2021
View reviewed changes
cmd/manager/main.go
// be propagated through the whole operator, generating
// uniform and structured logs.
logf.SetLogger(logf.ZapLogger(false))

Copy link
Contributor

Choose a reason for hiding this comment

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

why are we removing this? will it cause any logging format change in operator logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch - missed a local commit here

@amuraru amuraru force-pushed the build branch 3 times, most recently from e695e8e to 54ff34d Compare July 12, 2021 19:54
@anishakj
Copy link
Contributor

@amuraru Could you please verify operator upgrades are working fine?

anishakj
anishakj reviewed Jul 16, 2021
View reviewed changes
charts/zookeeper-operator/templates/_helpers.tpl
properties:
containerPort:
description: Number of port to expose on the pod's IP address.
This must be a valid port number, 0 < x < 65536.
format: int32
minimum: 1
maximum: 65535
Copy link
Contributor

@anishakj anishakj Jul 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

some of the openAPI Schema validations got removed. Could you please fix those? these were added additionally after generating CRD

@amuraru
Upgrade operator-sdk to 0.19
d07e2c2 
Upgraded operator-sdk: 0.17 -> 0.19

As part of this upgrade the following dependencies were also upgraded:
- Go 1.13 -> 1.16
- sigs.k8s.io/controller-runtime v0.5.2 -> v0.6.5
- k8s v0.17.5 -> v0.19.13
@amuraru amuraru mentioned this pull request Jul 17, 2021
Closed
anishakj
anishakj approved these changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@anishakj anishakj left a comment

Choose a reason for hiding this comment

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

LGTM

@anishakj anishakj merged commit 2369402 into pravega:master Jul 19, 2021
This was referenced Jul 19, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anishakj anishakj anishakj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Makefile is not providing a target to regenerate operator-sdk managed files
3 participants
@amuraru @codecov-commenter @anishakj