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 csiaddons to v0.5.0 & point volrep to csiaddons #534

Merged
merged 2 commits into from
Sep 9, 2022

Conversation

Rakshith-R
Copy link
Contributor

This commit updates csiaddons to v0.5.0 and other deps. This also points volrep to kubernetes-csi-addons repo now.

Signed-off-by: Rakshith R [email protected]

@Rakshith-R
Copy link
Contributor Author

/cc @rakeshgm @ShyamsundarR @Madhu-1

Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

PR is having formating changes and other go mod updates also. please try to keep the change minimal

@@ -313,17 +313,17 @@ func (u *drclusterInstance) finalizerRemove() error {
}

// TODO:
// 1) For now by default fenceStatus is ClusterFenceStateUnfenced.
Copy link
Member

Choose a reason for hiding this comment

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

This is not related to this PR please revert.

Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

LGTM expected few go.mod changes

Comment on lines +27 to +28
k8s.io/api v0.25.0
k8s.io/apimachinery v0.25.0
Copy link
Member

Choose a reason for hiding this comment

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

there are a few updates to use Kubernetes 0.25, not sure it can cause any problem, let @ShyamsundarR confirm on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is replaced below,

This automatically done when csi-addons was updated.

go.mod Outdated
k8s.io/apimachinery => k8s.io/apimachinery v0.23.6
k8s.io/client-go => k8s.io/client-go v0.23.6
k8s.io/kube-openapi => k8s.io/kube-openapi v0.0.0-20220124234850-424119656bbf
sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.11.2
Copy link
Member

Choose a reason for hiding this comment

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

Other than controller-runtime the other replace directives are to ensure ramen sticks to existing versions of these dependencies? Or, was this required for other reasons?

I typically do not want additional replace clauses, unless required. The controller-runtime is fine, so that we can catch up to the latest and remove the replace clause as time permits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The build and golangci is failing,
after normal
go get and go mod tidy

weirdly it is asking for login to github.ibm.com in my vscode, not sure why.

sigs.k8s.io/yaml v1.3.0 // indirect
)

replace k8s.io/client-go => k8s.io/client-go v0.23.6
replace k8s.io/client-go => k8s.io/client-go v0.25.0
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for clearing this section up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for clearing this section up.

It was a challenge figuring out what went wrong with the builds.
golangci linter needed update too in .github/workflow,
can you please trigger workflow again ?

This commit updates csiaddons to v0.5.0 and other deps.
This also points volrep to kubernetes-csi-addons repo now.

Signed-off-by: Rakshith R <[email protected]>
@Rakshith-R
Copy link
Contributor Author

https://github.com/RamenDR/ramen/runs/8248656188?check_suite_focus=true

go get does not work anymore,
changed it to go install

@Rakshith-R
Copy link
Contributor Author

Updated kustomize and handled the errors it threw
kubeflow/manifests#1797 (comment)

@Rakshith-R Rakshith-R requested review from ShyamsundarR and Madhu-1 and removed request for Madhu-1 and ShyamsundarR September 8, 2022 13:32
@Rakshith-R
Copy link
Contributor Author

@Madhu-1 @ShyamsundarR Please take a look
ci is green.

Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

changes LGTM, We need to update the build team to use go 1.18 to build ramen in downstream.

@@ -77,8 +77,8 @@ spec:
description: "Condition contains details for one aspect of the current
state of this API Resource. --- This struct is intended for direct
use as an array at the field path .status.conditions. For example,
type FooStatus struct{ // Represents the observations of a foo's
current state. // Known .status.conditions.type are: \"Available\",
\n type FooStatus struct{ // Represents the observations of a
Copy link
Member

Choose a reason for hiding this comment

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

Why were these new lines required? Or were these auto generated by make manifests?

- ../crd
- ../rbac
- ../manager
images:
Copy link
Member

Choose a reason for hiding this comment

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

Is this autogenerated?

@@ -58,3 +58,26 @@ linters:
- goheader # TODO: Introduce back post fixing linter errors
- gci
- interfacer # interfacer linter is archived and deprecated (https://github.com/mvdan/interfacer)
# TODO: fix folloing linter errors.
Copy link
Member

Choose a reason for hiding this comment

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

Are these new linters as compared to prior version v1.37.1 -> v1.49.0 of golang? If existing linters are being disabled, we would like to track and add them back, hence checking.

Copy link
Member

Choose a reason for hiding this comment

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

Typo folloing to following

@ShyamsundarR
Copy link
Member

Merging this as is, the linter changes we will look at and rectify if required.

@ShyamsundarR ShyamsundarR merged commit 2122d76 into RamenDR:main Sep 9, 2022
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.

3 participants