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

Closes #140 - Upgrading to kubebuilder format and SDK 1.0.1 #175

Merged
merged 3 commits into from
Oct 16, 2020

Conversation

ricardozanini
Copy link
Member

@ricardozanini ricardozanini commented Oct 9, 2020

Closes #140

TODO:

  • - Update documentation
  • - Migrate e2e tests
  • - Migrate hack scripts
  • - Migrate CI

😓

Signed-off-by: Ricardo Zanini [email protected]

@ricardozanini ricardozanini added this to the v0.4.0 milestone Oct 9, 2020
@ricardozanini ricardozanini added CI ⚙️ Continuous Integration Issue documentation 📖 Improvements or additions to documentation enhancement 👑 New feature or request Hacktoberfest hacktoberfest-accepted labels Oct 9, 2020
.vscode/launch.json Outdated Show resolved Hide resolved
@ricardozanini ricardozanini marked this pull request as draft October 10, 2020 17:21
@ricardozanini ricardozanini added the WIP 👷‍♀️ Work In Progress PR label Oct 10, 2020
@ricardozanini
Copy link
Member Author

@LCaparelli There's a lot of clutter in this PR. It's not ready yet. I forgot to change to draft, don't even waste your time reviewing it 😅😅

@ricardozanini
Copy link
Member Author

@LCaparelli @Kaitou786 @Kevin-Mok we're ready, I believe. If you guys have the time, please could you take a look? I'll get back at the end of the day to apply your suggestions, if any.

There's a lot to do, but I believe I covered the vast majority in this PR. We can keep improving. Since 0.4.0 will be released at the end of the month, I think we will have the time to find any bugs that might come up duo to the migration.

Copy link
Collaborator

@Kaitou786 Kaitou786 left a comment

Choose a reason for hiding this comment

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

They changed a lot but this is nice <3

Copy link
Member

@LCaparelli LCaparelli left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this @ricardozanini, looks like it was a handful. Also sorry for the delay.

Left some comments in

Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
controllers/nexus/resource/deployment/manager.go Outdated Show resolved Hide resolved
controllers/nexus/resource/networking/manager.go Outdated Show resolved Hide resolved
controllers/nexus/resource/networking/manager.go Outdated Show resolved Hide resolved
controllers/nexus/update/tags.go Outdated Show resolved Hide resolved
pkg/framework/fetcher.go Show resolved Hide resolved
pkg/framework/fetcher.go Outdated Show resolved Hide resolved
pkg/framework/fetcher.go Outdated Show resolved Hide resolved
pkg/test/client.go Show resolved Hide resolved
@ricardozanini
Copy link
Member Author

ricardozanini commented Oct 16, 2020

@LCaparelli I believe I've covered all your comments, can you take a look again please?

Copy link
Member

@LCaparelli LCaparelli left a comment

Choose a reason for hiding this comment

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

Thanks again @ricardozanini :-)

controllers/nexus/update/tags.go Show resolved Hide resolved
@@ -87,32 +87,32 @@ func (v *Validator) validateNetworking(nexus *v1alpha1.Nexus) error {
}

if !v.ingressAvailable && nexus.Spec.Networking.ExposeAs == v1alpha1.IngressExposeType {
v.log.Info("Ingresses are not available on your cluster. Make sure to be running Kubernetes > 1.14 or if you're running Openshift set ", "spec.networking.exposeAs", v1alpha1.IngressExposeType, "Also try", v1alpha1.NodePortExposeType)
v.log.Warn("Ingresses are not available on your cluster. Make sure to be running Kubernetes > 1.14 or if you're running Openshift set ", "spec.networking.exposeAs", v1alpha1.IngressExposeType, "Also try", v1alpha1.NodePortExposeType)
Copy link
Member

Choose a reason for hiding this comment

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

So you went with warnings, huh? Up to you, but I still think we should log it as error. Shouldn't cause too much trouble after #174 anyways

Copy link
Member Author

Choose a reason for hiding this comment

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

Take a look in the next line, you will see that there's an error being raised.

@ricardozanini ricardozanini merged commit c7484f1 into main Oct 16, 2020
@ricardozanini ricardozanini deleted the issue-140 branch October 16, 2020 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI ⚙️ Continuous Integration Issue documentation 📖 Improvements or additions to documentation enhancement 👑 New feature or request Hacktoberfest hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Operator SDK 1.0.0 and new Manifest Bundle
3 participants