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: add termination_message_policy to container schema (closes #442) #847

Merged

Conversation

AeroNotix
Copy link
Contributor

Description

Adds termination_message_policy to the container schema.

References

#442

@ghost ghost added the size/XS label May 17, 2020
@AeroNotix
Copy link
Contributor Author

@radeksimko please could I have a review on this PR.

@AeroNotix
Copy link
Contributor Author

@alexsomesan @pdecat @appilon any chance of a review?

Copy link
Contributor

@pdecat pdecat left a comment

Choose a reason for hiding this comment

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

Looks good, documentation for this should be added, see https://github.com/terraform-providers/terraform-provider-kubernetes/blame/v1.11.3/website/docs/r/pod.html.markdown#L338

Also, it's always nice to have acceptance tests for new attributes.

kubernetes/schema_container.go Outdated Show resolved Hide resolved
AeroNotix and others added 3 commits June 1, 2020 13:02
Co-authored-by: Patrick Decat <pdecat@gmail.com>
…Notix/terraform-provider-kubernetes into feat/add-termination-message-policy
@ghost ghost added the documentation label Jun 1, 2020
@AeroNotix
Copy link
Contributor Author

AeroNotix commented Jun 1, 2020

@pdecat where do the tests go?

@pdecat
Copy link
Contributor

pdecat commented Jun 1, 2020

You could probably introduce new test functions TestAccKubernetesPod_termination_message_policy_* to exercise the possible combinations (not set, set to File and set to FallbackToLogsOnError, like https://github.com/terraform-providers/terraform-provider-kubernetes/blob/v1.11.3/kubernetes/resource_kubernetes_pod_test.go#L294-L319

@ghost ghost added size/M and removed size/XS labels Jun 1, 2020
@AeroNotix
Copy link
Contributor Author

@pdecat added tests, didn't run them - hoping your CI runs them.

@aareet aareet added the theme/coverage issues that involve improving API coverage for the provider label Jun 2, 2020
@AeroNotix AeroNotix force-pushed the feat/add-termination-message-policy branch from 7d96432 to eb7a2e2 Compare June 12, 2020 11:34
@AeroNotix
Copy link
Contributor Author

@pdecat fixed up tests, can you take a look please.

@aareet aareet linked an issue Jun 24, 2020 that may be closed by this pull request
@AeroNotix
Copy link
Contributor Author

@pdecat ping? Apologies! Would like to get this merged if possible.

Copy link
Contributor

@pdecat pdecat left a comment

Choose a reason for hiding this comment

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

Hi @AeroNotix,

looks good overall, I left an question whether the new field should be computed or have a default. I'd like input from the provider maintainers here.

In addition to tests with pods, it would be nice to also have a multi-step test in the context of a deployment to validate updates of the field, e.g. step 0 no value, step 1 File, step 2 FallbackToLogsOnError.

The acceptance tests are currently failing because of a trivial typo I believe:

# make testacc TEST=./kubernetes TESTARGS='-run=TestAccKubernetesPod_termination_.* -count=1'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./kubernetes -v -run=TestAccKubernetesPod_termination_.* -count=1 -timeout 120m
=== RUN   TestAccKubernetesPod_termination_message_policy_default
--- PASS: TestAccKubernetesPod_termination_message_policy_default (16.25s)
=== RUN   TestAccKubernetesPod_termination_message_policy_override_as_file
--- PASS: TestAccKubernetesPod_termination_message_policy_override_as_file (6.85s)
=== RUN   TestAccKubernetesPod_termination_message_policy_override_as_fallback_to_logs_on_err
    TestAccKubernetesPod_termination_message_policy_override_as_fallback_to_logs_on_err: testing.go:654: Step 0 error: Check failed: 1 error occurred:
                * Check 2/2 error: kubernetes_pod.test: Attribute 'spec.0.container.0.termination_message_policy' expected "FallBackToLogsOnErr", got "FallbackToLogsOnError"


--- FAIL: TestAccKubernetesPod_termination_message_policy_override_as_fallback_to_logs_on_err (6.16s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-kubernetes/kubernetes 29.318s
FAIL
make: *** [GNUmakefile:17: testacc] Error 1

Running the acceptance tests locally with minikube is as easy as:

# minikube start
# export KUBE_CTX=minikube
# make testacc TEST=./kubernetes TESTARGS='-run=TestAccKubernetesPod_termination_.* -count=1'

Note: I'm just a contributor with no merge power here.

kubernetes/schema_container.go Outdated Show resolved Hide resolved
@AeroNotix
Copy link
Contributor Author

@aareet please can this be looked at

@aareet
Copy link
Member

aareet commented Jul 29, 2020

@AeroNotix thank you for this PR. It's already in our queue for review - we appreciate your patience. FAQ

@AeroNotix
Copy link
Contributor Author

Maybe you need to give active members of the community such as @pdecat commit access, lighten your heavy workload.

kubernetes/schema_container.go Outdated Show resolved Hide resolved
@AeroNotix
Copy link
Contributor Author

@jrhouston @pdecat moved over to setting the field as computed.

@AeroNotix AeroNotix force-pushed the feat/add-termination-message-policy branch from 99e1a4f to 508dcc0 Compare August 31, 2020 22:14
Copy link
Member

@aareet aareet left a comment

Choose a reason for hiding this comment

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

LGTM

❯ TESTARGS="-run '^TestAccKubernetesPod_termination_message_policy'" make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test "./kubernetes" -v -run '^TestAccKubernetesPod_termination_message_policy' -timeout 120m
=== RUN   TestAccKubernetesPod_termination_message_policy_default
--- PASS: TestAccKubernetesPod_termination_message_policy_default (15.33s)
=== RUN   TestAccKubernetesPod_termination_message_policy_override_as_file
--- PASS: TestAccKubernetesPod_termination_message_policy_override_as_file (8.04s)
=== RUN   TestAccKubernetesPod_termination_message_policy_override_as_fallback_to_logs_on_err
--- PASS: TestAccKubernetesPod_termination_message_policy_override_as_fallback_to_logs_on_err (18.04s)
PASS
ok  	github.com/terraform-providers/terraform-provider-kubernetes/kubernetes	43.415s

@aareet aareet merged commit a6f2273 into hashicorp:master Aug 31, 2020
@ghost
Copy link

ghost commented Oct 10, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Oct 10, 2020
@AeroNotix AeroNotix deleted the feat/add-termination-message-policy branch October 10, 2020 12:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation size/M theme/coverage issues that involve improving API coverage for the provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: termination_message_policy
4 participants