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

Provider client connection rework #34

Merged
merged 3 commits into from
Dec 9, 2024
Merged

Conversation

marnas
Copy link
Contributor

@marnas marnas commented Dec 5, 2024

Client connection has been reworked following new specifications.

Both Console and Gateway connection details have been consolidated and the usage of either is defined by the addition of a new variable named mode.

Most of the Client implementation has also been refactored in the process to avoid duplication.

@marnas marnas force-pushed the ops-594-gateway-client-rework branch 3 times, most recently from 924ca95 to bc92503 Compare December 5, 2024 19:45
Copy link
Member

@qboileau qboileau left a comment

Choose a reason for hiding this comment

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

for environment variables I think we should support the generic one like CDK_BASE_URL/CDK_ADMIN_USER/CDK_ADMIN_PASSWORD/... for both console and gateway and add fallback on specific one like CDK_CONSOLE_URL/CDK_GATEWAY_URL/CDK_CONSOLE_USER/CDK_CONSOLE_PASSWORD/CDK_GATEWAY_USER/... for the case when we have 2 instances at the same time and we don't want env var collision
WDYT ?

internal/provider/user_v2_resource.go Outdated Show resolved Hide resolved
templates/index.md.tmpl Outdated Show resolved Hide resolved
internal/provider/provider.go Show resolved Hide resolved
internal/provider/provider.go Show resolved Hide resolved
internal/provider/provider.go Outdated Show resolved Hide resolved
internal/provider/provider.go Outdated Show resolved Hide resolved
internal/provider/provider.go Outdated Show resolved Hide resolved
@marnas marnas force-pushed the ops-594-gateway-client-rework branch from bc92503 to 9314e69 Compare December 6, 2024 10:40
@marnas
Copy link
Contributor Author

marnas commented Dec 6, 2024

I agree with the additions proposed for the env variables but I would probably reverse the priority order from CDK_CONSOLE_URL/CDK_GATEWAY_URL first to then fall back on the generic CDK_BASE_URL.

The reasoning being to avoid collisions for someone who might get started with only one client and configure it with generic variables, to then add the other client with the relative specific without re-configuring the other client's variable.

Do you agree or would you rather enforce the usage of the specific variables in the case of multi client config?

@qboileau
Copy link
Member

qboileau commented Dec 6, 2024

I agree with the additions proposed for the env variables but I would probably reverse the priority order from CDK_CONSOLE_URL/CDK_GATEWAY_URL first to then fall back on the generic CDK_BASE_URL.

The reasoning being to avoid collisions for someone who might get started with only one client and configure it with generic variables, to then add the other client with the relative specific without re-configuring the other client's variable.

Do you agree or would you rather enforce the usage of the specific variables in the case of multi client config?

I'm good with this proposal. Because env var might not be used in first place and reply directly on terraform provider attributes directly removing the ambiguity.

@marnas marnas force-pushed the ops-594-gateway-client-rework branch from 26b140e to 2b24e00 Compare December 9, 2024 12:12
@marnas marnas force-pushed the ops-594-gateway-client-rework branch from 2b24e00 to 41f100a Compare December 9, 2024 14:23
Copy link
Member

@qboileau qboileau left a comment

Choose a reason for hiding this comment

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

LGTM

@marnas marnas merged commit a316da5 into main Dec 9, 2024
11 checks passed
@marnas marnas deleted the ops-594-gateway-client-rework branch December 9, 2024 14:28
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