hashicorp / terraform-provider-kubernetes

Terraform Kubernetes provider
https://www.terraform.io/docs/providers/kubernetes/
Mozilla Public License 2.0
1.58k stars 966 forks source link

Support `PersistentVolumeLastPhaseTransitionTime` feature gate field `lastPhaseTransitionTime` in `PersistentVolumeStatus` #2399

Closed BBBmau closed 1 month ago

BBBmau commented 8 months ago

Description

PersistentVolumeLastPhaseTransitionTime was recently graduated to beta and includes a new field as part of PersistentVolumeStatus

Potential Terraform Configuration

# Copy-paste your Terraform configurations here - for large Terraform configs,
# please use a service like Dropbox and share a link to the ZIP file. For
# security, you can also encrypt the files using our GPG public key.

References

Community Note

theloneexplorerquest commented 8 months ago

Hi, I am interested to work on this issue!

BBBmau commented 8 months ago

@theloneexplorerquest you're more then welcome to open a PR! We appreciate contributions from the community to improve the provider. We can assist you on PR if needed

theloneexplorerquest commented 7 months ago

Hi @BBBmau thanks for message,

From what I understand, for the actual code, I need to have an if-else statement at https://github.com/hashicorp/terraform-provider-kubernetes/blob/main/kubernetes/resource_kubernetes_persistent_volume_v1.go#L243 eg. log.Printf("[DEBUG] Persistent volume %s status received: %#v, lastPhaseTransitionTime is %v", out.Name, statusPhase, out.Status.Phase.lastPhaseTransitionTime)

My question is: do we want to output this attribute in terraform at all? Because I did not see PersistentVolumeStatus.message and PersistentVolumeStatus.reason as log.

I will make the code change and do some test too.

BBBmau commented 7 months ago

@theloneexplorerquest it makes sense to include both message and reason as part of the log. You can also add those along with lastPhaseTransitionTime.

and yes it looks like depending on the phase you'll want the log to output a different message. a failed status would include message and reason

theloneexplorerquest commented 7 months ago

Hi @BBBmau,

I have created the PR, can you take a look? For such small changes, is acceptance test required? I wasn't able to get it running with my Kops cluster yet. 😓

This is the error message I got for make testacc

=== NAME  xxx
    resource_xxx:24: Step 1/2 error: Error running apply: exit status 1

        Error: Failed to configure client: unable to load root certificates: unable to parse bytes as PEM block       

          with kubernetes_ingress_v1.test,
          on terraform_plugin_test.tf line 1, in resource "kubernetes_ingress_v1" "test":
           1: resource "kubernetes_ingress_v1" "test" {

Suspect I got some issues with ca certificate but wasn't able to identify it yet. if Acceptance test is required I will figure it out in the next few weeks

BBBmau commented 7 months ago

No need for a test since the stateConf doesn't have one already made, this is because it's a log output.