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

Fix examples so they compile and tests are green #1474

Merged
merged 1 commit into from Jan 9, 2021

Conversation

dsyer
Copy link
Contributor

@dsyer dsyer commented Jan 5, 2021

TBH I'm not sure why there are 3 sets of examples, or why they
are all in the same project/package. But there appears to have been
some rot because they are not included in CI. This attempts to fix
things.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 5, 2021
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 5, 2021
@@ -55,12 +54,12 @@
<dependency>
<groupId>io.kubernetes</groupId>
<artifactId>client-java-cert-manager-models</artifactId>
<version>0.16.1-SNAPSHOT</version>
<version>10.0.1</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these are the "head" examples, I think they should stay pointing at the head versions of these packages (e.g. locally built)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why are they 0.x.y then? I couldn't find those versions anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, this got overwritten here:
f68f177#diff-3b6e36d2658def9d7acba8e7dba313f73f237d0342df368a91fb84473dec8f9f

And I think I should revert it. The intent was that these would be versioned and released independent of the main client release.

I'll send a PR to revert these, and we can discuss the approach there.

</dependency>
<dependency>
<groupId>io.kubernetes</groupId>
<artifactId>client-java-prometheus-operator-models</artifactId>
<version>0.38.1-SNAPSHOT</version>
<version>10.0.1</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

here too.

@brendandburns
Copy link
Contributor

Thanks for doing this PR.

The theory here is that as the library evolves, and breaking changes get committed and the Kubernetes API changes, we have historically had a problem where people file an issue of the form:

"The example code doesn't work with old library version X"

So we have attempted to clone off the old examples for the old library versions.

e.g. release-10 -> 10.0.0, release-11 -> 11.0.0

And then we keep one that is pointed to "HEAD" (currently release-12)

This isn't necessarily the most awesome approach (and we're potentially open to others) but that is the rationale for the current state of affairs.

As you note, it is prone to bit rot (and also it totally broke the mvn release tooling last time I had to do a release) so improvements here are welcome!

@brendandburns
Copy link
Contributor

Oh, and that e2e test is still timing out as in #1418 I will investigate today.

@dsyer
Copy link
Contributor Author

dsyer commented Jan 5, 2021

The example code doesn't work with old library version X

Why wouldn't they just look at the tagged examples? Do we evolve new samples for old versions or the client (seems more onerous than it should be)?

I think what I would do is publish only examples that work with the current codebase, and also split them up so that they have better dependency management. You can probably fix the issue with the release plugin that way (and add skip=true to the deployment plugin for samples so they never get published as jars anyway).

@brendandburns
Copy link
Contributor

People just don't use the Tag UX in GitHub. The click to the repo, and then they click on the 'examples' directory.

I don't think we can expect people to be sophisticated enough to use the tag drop-down. (and empirical evidence from our issue queue confirms this)

@brendandburns
Copy link
Contributor

I was thinking about this some more, I think we should move these contrib/CRD examples into the contrib directory and remove these dependencies from the pom files. As I think about it more, I don't think it makes sense to include them in the main examples directory.

@yue9944882 wdyt?

@dsyer
Copy link
Contributor Author

dsyer commented Jan 7, 2021

Latest change bumps the version 12 examples to the latest contrib libs (currently 11.0.1-SNAPSHOT pending that PR that @brendanburns mentioned). Maybe we can merge this one and then move on from there to re-org the contrib samples into the contrib directory?

@yue9944882
Copy link
Member

I was thinking about this some more, I think we should move these contrib/CRD examples into the contrib directory and remove these dependencies from the pom files. As I think about it more, I don't think it makes sense to include them in the main examples directory.

@brendandburns i agree so. @dsyer can you rebase onto the latest master to make e2e test pass?

@dsyer dsyer force-pushed the examples branch 2 times, most recently from 401b102 to 02392d4 Compare January 8, 2021 10:25
Signed-off-by: Dave Syer <dsyer@vmware.com>
@dsyer
Copy link
Contributor Author

dsyer commented Jan 8, 2021

Force pushed to trigger re-build (it would be good to get rid of gmaven).

@brendandburns
Copy link
Contributor

/lgtm
/approve

(but I still think we should move the contrib examples out into the contrib directory)

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, dsyer

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 Jan 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit 3111c13 into kubernetes-client:master Jan 9, 2021
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. 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.

None yet

4 participants