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

Rollout improved example converter #650

Merged
merged 4 commits into from
Apr 17, 2024
Merged

Rollout improved example converter #650

merged 4 commits into from
Apr 17, 2024

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Apr 16, 2024

Rolls out improved TF example converter.

Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

Copy link

Is README.md missing any configuration options?

assumeRole not found in Configuration section
assumeRoleWithOidc not found in Configuration section
clientConnectTimeout not found in Configuration section
clientReadTimeout not found in Configuration section
configurationSource not found in Configuration section
credentialsUri not found in Configuration section
endpoints not found in Configuration section
fc not found in Configuration section
logEndpoint not found in Configuration section
maxRetryTimeout not found in Configuration section
mnsEndpoint not found in Configuration section
otsInstanceName not found in Configuration section
protocol not found in Configuration section
secureTransport not found in Configuration section
securityTransport not found in Configuration section
signVersion not found in Configuration section
sourceIp not found in Configuration section

Please add a description for each of these options to README.md. Details about them can be found in either the upstream docs or schema.json.

@iwahbe iwahbe self-requested a review April 17, 2024 10:59
@@ -31,72 +31,41 @@ namespace Pulumi.AliCloud.Cen
/// {
/// var config = new Config();
/// var anotherUid = config.Get("anotherUid") ?? "xxxx";
/// // Method 1: Use assume_role to operate resources in the target cen account, detail see https://registry.terraform.io/providers/aliyun/alicloud/latest/docs#assume-role
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we are losing some valuable comments here. Does the converter not support comments?

Comment on lines -32 to -40
/// var bj = new AliCloud.Provider("bj", new()
/// {
/// Region = "cn-beijing",
/// });
///
/// var hz = new AliCloud.Provider("hz", new()
/// {
/// Region = "cn-hangzhou",
/// });
Copy link
Member

Choose a reason for hiding this comment

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

We used to have multiple explicit providers here and we don't anymore? Any idea why?

Copy link
Member Author

Choose a reason for hiding this comment

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

@iwahbe
Copy link
Member

iwahbe commented Apr 17, 2024

I saw two things I have questions about, but otherwise this looks good.

@t0yv0 t0yv0 force-pushed the t0yv0/rollout-converter branch from 031fc86 to 66df704 Compare April 17, 2024 15:12
@t0yv0 t0yv0 force-pushed the t0yv0/rollout-converter branch from 66df704 to 40cc608 Compare April 17, 2024 15:13
@t0yv0 t0yv0 enabled auto-merge (squash) April 17, 2024 15:25
@t0yv0 t0yv0 merged commit 6f1cc6b into master Apr 17, 2024
16 checks passed
@t0yv0 t0yv0 deleted the t0yv0/rollout-converter branch April 17, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants