Skip to content
This repository has been archived by the owner on Feb 6, 2024. It is now read-only.

Add resolver to k8s_defaults #200

Merged
merged 6 commits into from
Nov 21, 2018
Merged

Conversation

samschlegel
Copy link
Contributor

This adds support for setting the default resolver with the k8s_defaults rule

This adds support for setting the default resolver with the `k8s_defaults` rule
@chrislovecnm
Copy link
Contributor

/ok-to-test

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Do we need to update the docs and can we add a test for this?

@chrislovecnm
Copy link
Contributor

Thanks for the PR!!! Appreciate your help.

@chrislovecnm
Copy link
Contributor

chrislovecnm commented Oct 23, 2018

This failed in e2e, and I am not certain why.

customresourcedefinition.apiextensions.k8s.io "todos.rules-k8s.bazel.io" deleted
Error from server: grpc: the client connection is closing

@samschlegel e2e failed, so I am rerunning it.

@chrislovecnm
Copy link
Contributor

/test pull-rules-k8s-e2e

@chrislovecnm
Copy link
Contributor

@nlopezgi any idea why the other bazel builds are not running??

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Passing CI now. Documentation and tests are the only thing we need 😊

@nlopezgi
Copy link
Contributor

Writing tests for this might be hard as these are workspace repo rules being changed, but lets just double check documentation is up to date and this should be good to submit imo

@fejta
Copy link
Contributor

fejta commented Oct 24, 2018

Writing tests for this might be hard

https://github.com/bazelbuild/rules_k8s/blob/master/examples/hellogrpc/BUILD#L20 etc use k8s_defaults rules... we could just put something to resolve in one of them, and ensure e2e tests continue passing

@chrislovecnm
Copy link
Contributor

Because tests are hard we can do another PR, but frankly we need a lot more tests 😞

@chrislovecnm
Copy link
Contributor

Can you rebase and do we need any doc changes?

Thanks for your help!

@chrislovecnm
Copy link
Contributor

@nlopezgi do we need docs update on this??

@k8s-ci-robot k8s-ci-robot removed the lgtm label Nov 14, 2018
@samschlegel
Copy link
Contributor Author

Apologies, I didn't see the updates to this. Let me know if there's anything I need to do

@chrislovecnm
Copy link
Contributor

chrislovecnm commented Nov 14, 2018

@samschlegel do we need any doc updates?

@nlopezgi can you start build kite CI?

/ok-to-test

@samschlegel
Copy link
Contributor Author

No doc updates are needed, as this is actually already in the docs as being supported 😅

@chrislovecnm
Copy link
Contributor

Need CI to pass and we can merge this

@nlopezgi
Copy link
Contributor

kicked off presubmits, this PR looks good other than the missing doc updates.

@samschlegel
Copy link
Contributor Author

samschlegel commented Nov 14, 2018

AFAICT no doc updates are needed as this was (erroneously?) added to docs in dc3999f

EDIT: Can add to the k8s_object/k8s_objects docs since I think that's what dc3999f meant to do

It has erroneously been in the k8s_defaults section since dc3999f which I believe was probably unintentional. This adds it to the k8s_object section
@nlopezgi
Copy link
Contributor

ugh, mac builds are timing out pulling images, will have to figure out tomorrow what's the problem

@chrislovecnm
Copy link
Contributor

@nlooezgi can you get this to retest?

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

We need another rebase.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, fejta, samschlegel

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

@chrislovecnm chrislovecnm merged commit 02fb100 into bazelbuild:master Nov 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants