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.

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

Update operator-sdk to 1.3.0 #86

Merged
merged 3 commits into from
Feb 4, 2021
Merged

Conversation

Mmduh-483
Copy link
Contributor

@Mmduh-483 Mmduh-483 commented Jan 20, 2021

Fixes #33

@Mmduh-483 Mmduh-483 changed the title [WIP] Update operator-sdk to 1.3.0 Update operator-sdk to 1.3.0 Jan 24, 2021
@Mmduh-483 Mmduh-483 force-pushed the sdk-1.3 branch 3 times, most recently from f01dcec to 28828bc Compare January 24, 2021 10:31
@abdallahyas
Copy link
Contributor

/retest-all

@Mmduh-483 Mmduh-483 force-pushed the sdk-1.3 branch 11 times, most recently from 16b8988 to 44ce70a Compare January 25, 2021 06:52
@adrianchiris
Copy link
Collaborator

Can you rebase your PR so conflicts are resolved ?

Generally it seems ok however a few issues i encountered;

  1. we should not maintain duplicated crds under deploy/ and under config/crd
  2. example folder should have up2date crds taken from config/crd or better yet remove them and update deploy/delete scripts to deploy the crds directly from their original directory WDYT?
  3. make image fails to build in my setup, does it work in yours ?
  4. i noticed a few targets added in Makefile, do they work ?

also a general question:
did you use this migration guide ?

@Mmduh-483
Copy link
Contributor Author

Mmduh-483 commented Jan 25, 2021

Can you rebase your PR so conflicts are resolved ?

udated

  1. we should not maintain duplicated crds under deploy/ and under config/crd

not duplicated but auto-generated, as discussed it will be removed in a new commit and update the example

  1. example folder should have up2date crds taken from config/crd or better yet remove them and update deploy/delete scripts to deploy the crds directly from their original directory WDYT?

it will be updated in new commit

  1. make image fails to build in my setup, does it work in yours ?

The issue is related to the new test in controllers/suite_test.go, it fails in the source command because the SHELL default is /bin/sh which doesn't contain source (I used . but still fails with other errors if you check the latest ci failure)
/bin/bash doesn't exist in the docker image golang/alpine
currentlly trying to fix this issue

  1. i noticed a few targets added in Makefile, do they work ?

yes

also a general question:
did you use this migration guide ?

yes

@Mmduh-483 Mmduh-483 force-pushed the sdk-1.3 branch 2 times, most recently from 6a175f4 to 3401738 Compare January 25, 2021 20:25
- Move pkg/api and pkg/controller and cmd/manager/main.go to project
root and update them according to new sdk structure
- Update Makefile with new rules
- Move build/Dockerfile project root
- Add new auto generated config dir

Signed-off-by: Mamduh Alassi <mamduhala@mellanox.com>
@adrianchiris
Copy link
Collaborator

@Mmduh-483 i see nic_operator CI is failing

@Mmduh-483
Copy link
Contributor Author

@Mmduh-483 i see nic_operator CI is failing

@AbdYsn need to adjust the new updates since this PR replace the template in deploy by example/deploy

@adrianchiris
Copy link
Collaborator

adrianchiris commented Feb 3, 2021

So i have reviewed this
a few nits and we are good to merge once CI passes:

  1. rename deploy.sh under scripts to docker-push.sh or similar as its confusing with deploy-operator.sh found there as well
  2. remove hack folder as it contains no meaningful info
  3. README.md L231 and L233 replace "Template" with "Example"
  4. example/README.md update Content section as it no longer contains the deploy/delete-operator
  5. example/README.md update setup cluster section, make deploy does not work when you are in example folder (change command to make -C ./.. deploy

abdallahyas pushed a commit to abdallahyas/kubernetes-ci that referenced this pull request Feb 3, 2021
In Mellanox/network-operator#86, the network operator updates the
operator SDK to v1.3, which break the CI. This patch fixes the break
in the CI and support the new and old ways of deploying the operator.
abdallahyas added a commit to abdallahyas/kubernetes-ci that referenced this pull request Feb 3, 2021
In Mellanox/network-operator#86, the network operator updates the
operator SDK to v1.3, which break the CI. This patch fixes the break
in the CI and support the new and old ways of deploying the operator.
abdallahyas added a commit to abdallahyas/kubernetes-ci that referenced this pull request Feb 3, 2021
In Mellanox/network-operator#86, the network operator updates the
operator SDK to v1.3, which break the CI. This patch fixes the break
in the CI and support the new and old ways of deploying the operator.
- Move move example/delete-operator.sh example/deploy-operator.sh to scripts/
- Read CRDs from config/crd/bases
- Update README

Signed-off-by: Mamduh Alassi <mamduhala@mellanox.com>
Signed-off-by: Mamduh Alassi <mamduhala@mellanox.com>
@Mmduh-483
Copy link
Contributor Author

Mmduh-483 commented Feb 3, 2021

So i have reviewed this
a few nits and we are good to merge once CI passes:

  1. rename deploy.sh under scripts to docker-push.sh or similar as its confusing with deploy-operator.sh found there as well
  2. remove hack folder as it contains no meaningful info
  3. README.md L231 and L233 replace "Template" with "Example"
  4. example/README.md update Content section as it no longer contains the deploy/delete-operator
  5. example/README.md update setup cluster section, make deploy does not work when you are in example folder (change command to make -C ./.. deploy

@adrianchiris Updated the PR except for 2, hack has the boilerplate.go.txt which contain the header you want to be added to the auto-generated go files, in our case the license, please check the rule generate in Makefile 194

@abdallahyas
Copy link
Contributor

/retest-nic_operator

@adrianchiris adrianchiris merged commit 9dc1d06 into Mellanox:master Feb 4, 2021
@moshe010 moshe010 mentioned this pull request Mar 1, 2021
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Update to operator-SDK v1.X
3 participants