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

Arbitrary arguments for k8s_objects #225

Merged
merged 2 commits into from
Nov 16, 2018

Conversation

samschlegel
Copy link
Contributor

@samschlegel samschlegel commented Nov 14, 2018

This builds on #224 by passing through arguments to the individual runs

What would be the best way to test this?

@k8s-ci-robot
Copy link

Hi @samschlegel. Thanks for your PR.

I'm waiting for a bazelbuild member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@nlopezgi
Copy link
Contributor

Hi @samschlegel ,
thanks for sending this PR.
To test this you could add to https://github.com/bazelbuild/rules_k8s/blob/master/examples/todocontroller/e2e-test.sh the same functionality like the one I added in #224 to https://github.com/bazelbuild/rules_k8s/blob/master/examples/hellohttp/e2e-test.sh to perform checks. Then add a test like https://github.com/bazelbuild/rules_k8s/blob/master/examples/hellohttp/e2e-test.sh#L118 in https://github.com/bazelbuild/rules_k8s/blob/master/examples/todocontroller/e2e-test.sh that runs "bazel run examples/todocontroller: everything -- -v=2" and checks the command(s) executed (should be printed as a result of the changes in #224 in the tpl files) do(es) contain the -v=2 arg as you expect

@chrislovecnm
Copy link
Contributor

/ok to test

@chrislovecnm
Copy link
Contributor

/ok-to-test

@chrislovecnm
Copy link
Contributor

@samschlegel to add to what Nick mentioned. Please do not enable the controller tests. We have disabled those till we work some problems out of them. You can do the run everything on the other examples.

@samschlegel
Copy link
Contributor Author

So I should add them to todocontroller, but not enable the tests?

@samschlegel
Copy link
Contributor Author

samschlegel commented Nov 14, 2018

It looks like hellogrpc also uses k8s_objects, so I could do it there instead.

@chrislovecnm
Copy link
Contributor

@samschlegel a grpc test would be great!

@samschlegel
Copy link
Contributor Author

Test added!

@chrislovecnm
Copy link
Contributor

Can you rebase?

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.

Test comment

# Checks that bazel run <some target> -- <some extra arg> does pass both the
# args in the attr as well as the <some extra arg> to the execution of the
# template
EXPECT_CONTAINS "$(bazel run examples/hellogrpc:staging.apply -- --v=1)" "apply --v=2 --v=1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is your test, but we have two —v on it

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 just copied how @nlopezgi did the e2e test in hellohttp

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I dont know much about what valid args to pass, this is just testing that the arg set in the build file and the one set in the bazel run command both get passed through to the script, this doesnt really test the args actually do something meaningful

@nlopezgi
Copy link
Contributor

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

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nlopezgi, 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

@nlopezgi nlopezgi merged commit 588652b into bazelbuild:master Nov 16, 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

4 participants