Updated optional component List to possible values #5136

sehgalnamit · 2021-08-25T03:51:02Z

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
-[X] Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
-[X] Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

Refer:- https://cloud.google.com/dataproc/docs/reference/rest/v1/ClusterConfig#Component Issue Link hashicorp/terraform-provider-google#9766

modular-magician · 2021-08-25T03:52:27Z

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@c2thorn, please review this PR or find an appropriate assignee.

modular-magician · 2021-08-25T04:02:03Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 2 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 2 files changed, 5 insertions(+), 4 deletions(-))

sehgalnamit · 2021-08-25T04:14:57Z

@c2thorn
Many Thanks for getting this request reviewed.
May I know what this check failure is and any actions I need to do ?

TerraformVCRCommunity

c2thorn

Hi @sehgalnamit, just so we're on the same page, the logic you are updating only affects tests correct?
The schema for the actual resource appears in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/mmv1/third_party/terraform/resources/resource_dataproc_cluster.go.erb#L542

sehgalnamit · 2021-08-26T01:32:55Z

@c2thorn
I already checked that code you have pointed already has the FLINK component and it does not work when we use terraform resource with optional component FLINK.
There is also a feature request raised for this 1173452908414427

This is my actual issue
hashicorp/terraform-provider-google#9766

I was guided to use different codes and also I got another update with below:-

#5129

Possibly https://github.com/GoogleCloudPlatform/magic-modules/blob/master/mmv1/third_party/terraform/tests/resource_dataproc_cluster_test.go.erb

Thanks

sehgalnamit · 2021-09-01T07:57:46Z

@c2thorn Can you please advise on this?
I am not sure where do I need to commit to make this change work?

Thanks

c2thorn · 2021-09-02T19:06:08Z

@sehgalnamit
Ah I think I see what's going on here. regarding #5129 (review), they were stating that you'll want to make changes in all 3 of those files in the same pull request. You've done 1/3 here by changing the test file

sehgalnamit · 2021-09-03T01:47:44Z

@c2thorn
Many Thanks for your reply, it is great that you guys reply on community so fast.
If you see all those 3 files, we already have the FLINK component added and the only file which was missing, I added it there.

My goal is to get FLINK component added in the provider:-
https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/dataproc_cluster

optional_components -

c2thorn · 2021-09-07T14:03:03Z

@sehgalnamit I see, that makes sense. I still do not see FLINK added as an option to the documentation: https://github.com/GoogleCloudPlatform/magic-modules/blob/master/mmv1/third_party/terraform/website/docs/r/dataproc_cluster.html.markdown?plain=1#L487

We could add that in this PR?

sehgalnamit · 2021-09-08T02:45:21Z

@c2thorn
Yes, please

modular-magician · 2021-09-08T02:49:54Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 3 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 3 files changed, 4 insertions(+), 3 deletions(-))

c2thorn · 2021-09-08T16:11:34Z

/gcbrun

modular-magician · 2021-09-08T16:22:41Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 3 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 3 files changed, 4 insertions(+), 3 deletions(-))

c2thorn · 2021-09-08T20:29:23Z

Hi @sehgalnamit
Running the tests and then looking at it manually, there are a few further issues.

ANACONDA, KERBEROS, and RANGER optional components all seem to have additional requirements needed to be valid values, and cause the API to return a 400 error.

Even after removing these, the test check to verify optional components list does not do an un-ordered comparison:

magic-modules/mmv1/third_party/terraform/tests/resource_dataproc_cluster_test.go.erb

Line 904 in 174ad27

    
           if !reflect.DeepEqual(components, cluster.Config.SoftwareConfig.OptionalComponents) {

If the list is given in any order not specified in

magic-modules/mmv1/third_party/terraform/tests/resource_dataproc_cluster_test.go.erb

Line 575 in 174ad27

    
           testAccCheckDataprocClusterHasOptionalComponents(&cluster, "ZOOKEEPER", "DOCKER", "ANACONDA", "DRUID", "HBASE", "FLINK", "SOLR", "ZEPPELIN", "HIVE_WEBHCAT", "JUPYTER", "KERBEROS", "PRESTO", "RANGER"),

, it will fail the test.

Given these issues, I don't think it is worth actually testing every possible value for optional components. It isn't something we do for other fields, and given the number of possible values it should be fine to just test DOCKER and ZOOKEEPER (the original values in the test). What do you think? If you agree, we can just keep the HTML change, revert the test changes, and merge the PR.

sehgalnamit · 2021-09-09T11:41:43Z

@c2thorn
Really appreciate your help on this case.
I am fine to have HTML changes but is it possible for you to test atleast FLINK as component as when I use the provider in my environment, it does not work.

https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/dataproc_cluster

terraform {
required_providers {
google = {
source = "hashicorp/google"
version = "3.82.0"
}
}
}

provider "google" {

Configuration options

}

It could be Google API issue with Flink which I can go to Google Product team and ask. If you get similar issue for FLINK as an optional component not working while spinning of dataproc using this provider, I do not think it is worth to update HTML.
There is a feature request I have raised with heshicorp but I think they also depend on open source community.

My original issue
hashicorp/terraform-provider-google#9766

c2thorn · 2021-09-09T20:22:16Z

Hi @sehgalnamit, this makes sense I agree with at least adding FLINK to one of the tested values.

Trying it out myself very quickly, I see success. If you are having problems, can you post your config and Debug log?

sehgalnamit · 2021-09-13T08:24:22Z

@c2thorn
Thanks for your reply.
I managed to add FLINK as addental component
dataproc_clusters = {
"welltuned" : {
software_config_optional_components = ["ZOOKEEPER", "JUPYTER", "FLINK"]
}
}

So you can just update HTML for the PR.

I found further issues with though:-

If you get time, please have a look at this
https://github.com/GoogleCloudDataproc/initialization-actions/tree/master/flink

NOTE: The Flink initialization action has been deprecated. Please use the Flink Component
Even though we have an option to add FLINK as additional component, we do not have a way to start the FLINK session, something we have in gcloud command

flink-start-yarn-session=true
https://cloud.google.com/dataproc/docs/concepts/components/flink

If we rerun the code,
IP of the nodes and master will not preserved

Meaning I get new IPs even though in my plan, I see no change in the resource.

c2thorn · 2021-09-14T19:46:18Z

So you can just update HTML for the PR.

Do you mind reverting the change to the test file in this PR, then I'll merge

Even though we have an option to add FLINK as additional component, we do not have a way to start the FLINK session, something we have in gcloud command

flink-start-yarn-session=true
cloud.google.com/dataproc/docs/concepts/components/flink

We are limited by the API in what we can do, and there is not always parity with gcloud. I cannot find anything here: https://cloud.google.com/dataproc/docs/reference/rest/v1/projects.regions.clusters

Meaning I get new IPs even though in my plan, I see no change in the resource.

Is this a current bug in the resource? It may be appropriate to open a new issue with the steps involved so we can solve it. I believe this is falling out of the scope of this PR.

modular-magician · 2021-09-15T02:27:47Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 1 insertion(+))
Terraform Beta: Diff ( 2 files changed, 2 insertions(+), 2 deletions(-))

sehgalnamit · 2021-09-15T06:06:07Z

@c2thorn
I have raised an issue for the IPs issue
hashicorp/terraform-provider-google#10074

Also, I have raised a Case with Google for REST API issue

Updated my current PR.

c2thorn · 2021-09-15T06:33:22Z

/gcbrun

modular-magician · 2021-09-15T06:43:43Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 1 insertion(+))
Terraform Beta: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2021-09-15T08:18:03Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccComputeForwardingRule_internalHttpLbWithMigBackendExample|TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample|TestAccComputeForwardingRule_forwardingRuleExternallbExample|TestAccComputeForwardingRule_forwardingRuleGlobalInternallbExample|TestAccComputeForwardingRule_forwardingRuleL3DefaultExample|TestAccComputeForwardingRule_forwardingRuleInternallbExample|TestAccComputeForwardingRule_forwardingRuleHttpLbExample|TestAccComputePacketMirroring_computePacketMirroringFullExample|TestAccComputeRoute_routeIlbExample|TestAccComputeServiceAttachment_serviceAttachmentBasicExample|TestAccComputeServiceAttachment_serviceAttachmentExplicitProjectsExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=205931

modular-magician · 2021-09-15T08:34:10Z

Tests failed during RECORDING mode: TestAccComputeServiceAttachment_serviceAttachmentBasicExample Please fix these to complete your PR

sehgalnamit · 2021-09-17T01:24:43Z

@c2thorn

You can merge the HTML.

…rm#5136)

Updated optional component List to possible values

c036c48

Refer:- https://cloud.google.com/dataproc/docs/reference/rest/v1/ClusterConfig#Component Issue Link hashicorp/terraform-provider-google#9766

google-cla bot added the cla: yes label Aug 25, 2021

modular-magician requested a review from c2thorn August 25, 2021 03:52

c2thorn reviewed Aug 25, 2021

View reviewed changes

Updating FLINK as an optional component

174ad27

This comment has been minimized.

Sign in to view

sehgalnamit added 2 commits September 15, 2021 10:14

Reverting changes back.

11c9ebc

reverting changes

8473c3d

c2thorn approved these changes Sep 17, 2021

View reviewed changes

c2thorn merged commit c58e492 into GoogleCloudPlatform:master Sep 17, 2021

This was referenced Sep 17, 2021

Updated optional component List to possible values hashicorp/terraform-provider-google-beta#3632

Merged

Updated optional component List to possible values hashicorp/terraform-provider-google#10097

Merged

melinath mentioned this pull request Sep 22, 2021

Changing optionalComponents from enum to String #5129

Closed

khajduczenia pushed a commit to khajduczenia/magic-modules that referenced this pull request Oct 12, 2021

Updated optional component List to possible values (GoogleCloudPlatfo…

849f7f0

…rm#5136)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated optional component List to possible values #5136

Updated optional component List to possible values #5136

sehgalnamit commented Aug 25, 2021 •

edited by c2thorn

Loading

modular-magician commented Aug 25, 2021

modular-magician commented Aug 25, 2021

sehgalnamit commented Aug 25, 2021

c2thorn left a comment

sehgalnamit commented Aug 26, 2021

sehgalnamit commented Sep 1, 2021

c2thorn commented Sep 2, 2021

sehgalnamit commented Sep 3, 2021

c2thorn commented Sep 7, 2021

sehgalnamit commented Sep 8, 2021

modular-magician commented Sep 8, 2021

c2thorn commented Sep 8, 2021

modular-magician commented Sep 8, 2021

This comment has been minimized.

c2thorn commented Sep 8, 2021

sehgalnamit commented Sep 9, 2021

c2thorn commented Sep 9, 2021

sehgalnamit commented Sep 13, 2021

c2thorn commented Sep 14, 2021 •

edited

Loading

modular-magician commented Sep 15, 2021

sehgalnamit commented Sep 15, 2021

c2thorn commented Sep 15, 2021

modular-magician commented Sep 15, 2021

modular-magician commented Sep 15, 2021

modular-magician commented Sep 15, 2021

sehgalnamit commented Sep 17, 2021

Updated optional component List to possible values #5136

Updated optional component List to possible values #5136

Conversation

sehgalnamit commented Aug 25, 2021 • edited by c2thorn Loading

modular-magician commented Aug 25, 2021

modular-magician commented Aug 25, 2021

Diff report:

sehgalnamit commented Aug 25, 2021

c2thorn left a comment

Choose a reason for hiding this comment

sehgalnamit commented Aug 26, 2021

sehgalnamit commented Sep 1, 2021

c2thorn commented Sep 2, 2021

sehgalnamit commented Sep 3, 2021

c2thorn commented Sep 7, 2021

sehgalnamit commented Sep 8, 2021

modular-magician commented Sep 8, 2021

Diff report:

c2thorn commented Sep 8, 2021

modular-magician commented Sep 8, 2021

Diff report:

This comment has been minimized.

c2thorn commented Sep 8, 2021

sehgalnamit commented Sep 9, 2021

Configuration options

c2thorn commented Sep 9, 2021

sehgalnamit commented Sep 13, 2021

So you can just update HTML for the PR.

c2thorn commented Sep 14, 2021 • edited Loading

modular-magician commented Sep 15, 2021

Diff report:

sehgalnamit commented Sep 15, 2021

c2thorn commented Sep 15, 2021

modular-magician commented Sep 15, 2021

Diff report:

modular-magician commented Sep 15, 2021

modular-magician commented Sep 15, 2021

sehgalnamit commented Sep 17, 2021

sehgalnamit commented Aug 25, 2021 •

edited by c2thorn

Loading

c2thorn commented Sep 14, 2021 •

edited

Loading