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 Tools and API Versions #56

Merged
merged 6 commits into from
Jul 3, 2023

Conversation

razo7
Copy link
Member

@razo7 razo7 commented Jun 12, 2023

Bump FAR to the latest tool and API versions (e.g. K8s, OCP, Controller-gen, Kustomize, and Operator-SDK).

In addition it also includes more stuff:

  • Fix for writing/deleting the envtest target directory
  • Fetching the machine client (for E2E) using OpenShift client-go

ECOPROJECT-1403

@openshift-ci openshift-ci bot requested review from beekhof and slintes June 12, 2023 06:15
@razo7
Copy link
Member Author

razo7 commented Jun 12, 2023

Golang must be v1.20 prior to merging this PR.
/hold
Wait for #57 to be merged

@razo7 razo7 force-pushed the update-tools-v0.1.0+ branch 3 times, most recently from f1c58a0 to 144013d Compare June 14, 2023 08:18
@razo7
Copy link
Member Author

razo7 commented Jun 14, 2023

E2E test will fail as it needs openshift/release#40248 to be merged, so Golang version on E2E will be updated

@razo7
Copy link
Member Author

razo7 commented Jun 14, 2023

/retest

@razo7 razo7 changed the title Update Tool and API Versions Update Tools and API Versions Jun 15, 2023
@mshitrit
Copy link
Member

/lgtm
/hold
Giving others chance to review

@clobrano
Copy link
Contributor

/lgtm

Giving a chance to get more feedbacks, feel free to unhold
/hold

@razo7
Copy link
Member Author

razo7 commented Jun 25, 2023

I hope #20 will be merged soon, and I prefer to wait for this PR to be merged. Since the current PR introduce huge bump (by time of versions, and not necessarily of FAR's functionality), thus I would prefer to wait for #20.

@razo7
Copy link
Member Author

razo7 commented Jun 28, 2023

/retest

@razo7
Copy link
Member Author

razo7 commented Jun 28, 2023

E2E PR was merged and this PR is ready for review

Copy link
Member

@slintes slintes left a comment

Choose a reason for hiding this comment

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

there changes not relevant to the PR title in the PR

  • please revert the kustomize changes and put them in into their own PR. And when doing so, either delete all unneeded and/or commented configs, or migrate all of them to the new format. What we have in this PR now is a mix of everything. I'd prefer to migrate all (and keep them commented), it will make enabling webhooks later on much easier
  • there are also code changes, probably also worth their own PR?

@@ -73,5 +73,5 @@ status:
acceptedNames:
kind: ""
plural: ""
conditions: []
storedVersions: []
conditions: null
Copy link
Member

Choose a reason for hiding this comment

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

interesting... in /config this status is removed completely from the CRDs, here just the list values change 🤔 🤷

@@ -6,7 +6,6 @@ resources:
- bases/fence-agents-remediation.medik8s.io_fenceagentsremediationtemplates.yaml
#+kubebuilder:scaffold:crdkustomizeresource

patchesStrategicMerge:
Copy link
Member

Choose a reason for hiding this comment

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

so... how would we enable webhooks now?

@@ -45,31 +37,15 @@ patchesStrategicMerge:
#- webhookcainjection_patch.yaml

# the following config is for teaching kustomize how to do var substitution
vars:
# [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER' prefix.
Copy link
Member

Choose a reason for hiding this comment

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

not sure if it's a good idea to remove this certmanager config...

@razo7
Copy link
Member Author

razo7 commented Jun 29, 2023

Ok, I will create PR for changing the OCP client-go and fetching machine client in E2E test and in another PR I will update and fix deprecation warnings due to Kustomize v5 changes.

migrate all of them to the new format.

When you say new format are you referring to Kubebuilder go/v3 to g0/v4? I remembered that go/v4 was still as alpha, and looking again I see that it has already been stabilized ECOPROJECT-1315

There are newer versions & reorganize the tool order as their targets order
There was no write permission, thus go-install-tool couldn't remove the old directory
@razo7
Copy link
Member Author

razo7 commented Jun 29, 2023

there are also code changes, probably also worth their own PR?

I was thinking of doing another PR for them.

Ok, I will create PR for changing the OCP client-go and fetching machine client in E2E test

But looking again on the changes, they are quite minor IMHO, and due to bumping the K8s and OCP APIs I was aware of an error with vet under vendor due to the old github.com/openshift/machine-api-operator package.
Therefore, I would prefer to let the these code changes remain in this PR.
I can update the PR's title if you still see a need for that.
WDYT?

@slintes
Copy link
Member

slintes commented Jun 29, 2023

there are also code changes, probably also worth their own PR?

I was thinking of doing another PR for them.

Ok, I will create PR for changing the OCP client-go and fetching machine client in E2E test

But looking again on the changes, they are quite minor IMHO, and due to bumping the K8s and OCP APIs I was aware of an error with vet under vendor due to the old github.com/openshift/machine-api-operator package. Therefore, I would prefer to let the these code changes remain in this PR. I can update the PR's title if you still see a need for that. WDYT?

ack, wfm

Use Kubernetes v1.27, and OCP v4.14 (go get -u github.com/openshift/[email protected])
github.com/openshift/machine-api-operator pacakge was outdated, thus we use the OCP client-go package
Add missing and remove unused modules, make vendored copy of dependencies, and verify dependencies have expected content
@razo7
Copy link
Member Author

razo7 commented Jun 29, 2023

Updating to kustomize v5, and removing new deprecation warnings will be done in a follow up PR when we change the project layout to go/v4 - ECOPROJECT-1315

@razo7
Copy link
Member Author

razo7 commented Jun 29, 2023

/unhold

@openshift-ci openshift-ci bot added the lgtm label Jul 3, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 3, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: razo7, slintes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 50ea936 into medik8s:main Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants