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

Updated Operator SDK to v0.11.0 #695

Merged

Conversation

jpkrohling
Copy link
Contributor

Closes #692.

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

@@ -5,7 +5,7 @@

if ! whoami &>/dev/null; then
if [ -w /etc/passwd ]; then
echo "${USER_NAME:-qaclana}:x:$(id -u):$(id -g):${USER_NAME:-qaclana} user:${HOME}:/sbin/nologin" >> /etc/passwd
echo "${USER_NAME:-jaeger-operator}:x:$(id -u):$(id -g):${USER_NAME:-jaeger-operator} user:${HOME}:/sbin/nologin" >> /etc/passwd
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 there since the very first commit, I suppose. In my initial PoC of the operator, I used a pet project (qaclana) to learn about the operator, and it looks like this was left behind when I started this operator here for real.

Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

SDK version has to be changed in makefile. The CI should use makefile to install the SDK to keep it consistent.

@@ -10,7 +10,7 @@ The Jaeger Operator is an implementation of a [Kubernetes Operator](https://kube
To install the operator, run:
```
kubectl create namespace observability
kubectl create -f https://raw.githubusercontent.com/jaegertracing/jaeger-operator/master/deploy/crds/jaegertracing_v1_jaeger_crd.yaml
kubectl create -f https://raw.githubusercontent.com/jaegertracing/jaeger-operator/master/deploy/crds/jaegertracing.io_jaegers_crd.yaml
Copy link
Member

Choose a reason for hiding this comment

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

This will probably break clients/tutorials. Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could probably keep the old one there. Unfortunately, this is a breaking change from the Operator SDK itself and some commands from it expect the file to be in this new location.


type matchingLabelKeys map[string]string

func (m matchingLabelKeys) ApplyToList(opts *client.ListOptions) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't cleaner to return labelSelector than modifying the struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would have been, but they changed the API and I couldn't find a cleaner way to do it. See operator-framework/operator-sdk#2060

@jpkrohling
Copy link
Contributor Author

The CI should use makefile to install the SDK to keep it consistent.

It's build this way to make use of the caching mechanism from the CI.

@jpkrohling
Copy link
Contributor Author

PR updated:

  1. keep the old CRD file (with a comment)
  2. fix the Operator SDK version in the Makefile

Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

Long live breaking changes!

@@ -1,3 +1,6 @@
# ATTENTION: this file is kept here for backwards compatibility reasons
# use the new location: jaegertracing.io_jaegers_crd.yaml . If there are
# inconsistencies between the files, this one is considered wrong.
Copy link
Member

Choose a reason for hiding this comment

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

+1 it would be cool to print deprecation message when it is used. Not sure if that is possible though.

@@ -6,7 +6,7 @@ import (
"testing"
"time"

"github.com/googleapis/gnostic/OpenAPIv2"
openapi_v2 "github.com/googleapis/gnostic/OpenAPIv2"
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think we don't use underscores in import names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this came from the make fmt, which puts the package name from the source file instead of the path.

https://github.com/googleapis/gnostic/blob/master/OpenAPIv2/OpenAPIv2.go#L17

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@jpkrohling jpkrohling force-pushed the 692-Update-Operator-SDK-to-0.11.0 branch from dec14c2 to 604485a Compare October 15, 2019 16:02
@jpkrohling jpkrohling merged commit bad90a6 into jaegertracing:master Oct 16, 2019
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.

Update Operator SDK to v0.11.0
2 participants