Skip to content

Conversation

@atoato88
Copy link
Contributor

@atoato88 atoato88 commented Apr 8, 2020

In PR #81, dashboard-operator is removed in this repository.
It is needed that change to use guestbook-operator in hack/smoketest.go instead of dashboard-operator.

related to: #80

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Apr 8, 2020
@k8s-ci-robot k8s-ci-robot requested review from droot and monopole April 8, 2020 06:23
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 8, 2020
Copy link
Contributor Author

@atoato88 atoato88 left a comment

Choose a reason for hiding this comment

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

Adds concerns in comment.


# Build the docker image
docker-build: test
sed -e "s:^replace://replace:g" go.mod > go.mod.make-docker-build
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 a rough approach, I think...
Without this change, go run smoketest.go will fail.
Here is log.

...
...
Step 5/14 : RUN go mod download
 ---> Running in 3e6c6d104896
go: sigs.k8s.io/kubebuilder-declarative-pattern@v0.0.0-20200317144824-bbf1fb2a4a9a: parsing /go.mod: open /go.mod: no such file or directory
The command '/bin/sh -c go mod download' returned a non-zero code: 1
Makefile:64: recipe for target 'docker-build' failed
make: *** [docker-build] Error 1

Any Ideas? or, is this reasonable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @atoato88 ! I'm trying to think of a different solution here, we may not have much of an option though. It's a tricky scenario to build correctly.

Copy link
Contributor

@johnsonj johnsonj Apr 13, 2020

Choose a reason for hiding this comment

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

The problem is that the build target is dependent on a library that is in the same source tree (both with their own go mod files). The separation is necessary because we don't want examples/.. to be part of the library when a user consumes this.

The dashboard-operator took the following approach:

  • Replace the kubebuilder-declarative-pattern dependency in the go.mod to a relative file path (../../): src
  • Use vendored dependencies (instead of go modules) in the docker build src
  • Update the vendor dependencies as part of docker-build src

This approach ensures that you are always running the latest code from the main repo when running the example. This is low maintenance, great for testing/development. It would 'feel better' if it didn't require using vendored dependencies. There may be a workaround for that, but I'm not sure.

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 helpful comment, I'll change my PR according to comment!! 😄

Copy link
Contributor Author

@atoato88 atoato88 Apr 14, 2020

Choose a reason for hiding this comment

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

I've fixed about this.
Your comment solved all concerns.
Thank you!!

# Copy the Go Modules manifests
COPY go.mod go.mod
#COPY go.mod go.mod
COPY go.mod.make-docker-build go.mod
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 relate to my comment at L64 in examples/guestbook-operator/Makefile

@atoato88
Copy link
Contributor Author

atoato88 commented Apr 8, 2020

/retitle Use guestbook-operator in smoketest

@k8s-ci-robot k8s-ci-robot changed the title WIP: Use guestbook-operator in smoketest Use guestbook-operator in smoketest Apr 8, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 8, 2020
@atoato88
Copy link
Contributor Author

atoato88 commented Apr 9, 2020

/assign @johnsonj

@johnsonj
Copy link
Contributor

Thank you @atoato88 !

/lgtm
/approve

kustomize build config/default | kubectl apply -f -

# Teardown controller in the configured Kubernetes cluster in ~/.kube/config
# This command does things that can't always re-run so the exit codes are ignored
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the comment here!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atoato88, johnsonj

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2020
@k8s-ci-robot k8s-ci-robot merged commit 92702a3 into kubernetes-sigs:master Apr 14, 2020
@atoato88 atoato88 deleted the modify-smoketest branch December 12, 2022 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants