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

Add original astro runtime version to deployment resource creation #58

Merged
merged 16 commits into from
May 15, 2024

Conversation

vandyliu
Copy link
Collaborator

@vandyliu vandyliu commented May 13, 2024

Description

  • adds original astro runtime version to deployment resource so customers can set their astro runtime version on deployment creation

🎟 Issue(s)

#astro

🧪 Functional Testing

  • Tested with original_astro_runtime_version set, with it not set and also importing an existing deployment

📸 Screenshots

📋 Checklist

  • Rebased from the main (or release if patching) branch (before testing)
  • Added/updated applicable tests
  • Added/updated examples in the examples/ directory
  • Tested against Astro-API
  • Communicated to/tagged owners of respective clients potentially impacted by these changes.
  • Updated any related documentation

@vandyliu vandyliu marked this pull request as ready for review May 15, 2024 00:38
@vandyliu vandyliu requested review from sunkickr and a team as code owners May 15, 2024 00:38
@@ -12,7 +12,7 @@ OAPI_CODEGEN ?= $(ENVTEST_ASSETS_DIR)/oapi-codegen
# Run acceptance tests
.PHONY: testacc
testacc:
TF_ACC=1 go test ./... -v -run TestAcc $(TESTARGS) -timeout 120m
TF_ACC=1 go test ./... -v -run TestAcc $(TESTARGS) -timeout 180m

Choose a reason for hiding this comment

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

Just curious about the increase here, is this due to the resource tests being added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screenshot 2024-05-14 at 10 21 02 PM
Seems like the tests take 2.5 hours to run (and pass) so I upped it

// Hybrid deployment specific fields
TaskPodNodePoolId types.String `tfsdk:"task_pod_node_pool_id"`

// Hosted deployment specific fields

Choose a reason for hiding this comment

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

nit: since you mentioned dedicated above, so that we don't have to infer what you mean by hosted below

Suggested change
// Hosted deployment specific fields
// Hosted (shared and dedicated) deployment specific fields

Choose a reason for hiding this comment

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

Non-blocking

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will add

return diags
}
data.Executor = types.StringPointerValue((*string)(deployment.Executor))
if deployment.SchedulerAu != nil {

Choose a reason for hiding this comment

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

Since we're starting by reading common fields, should we move this lower, in the hybrid only section? Perhaps with the TaskPodNodePoolId? Might've missed it, but this comment is just based off how you re-organized the deployment resource fields to read from the response!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do

@vandyliu vandyliu changed the title Add astro runtime version to deployment resource v2 Add original astro runtime version to deployment resource creation May 15, 2024
@vandyliu vandyliu merged commit e9d620a into main May 15, 2024
5 checks passed
@vandyliu vandyliu deleted the add-astro-runtime-version-to-deployment-resource-v2 branch May 15, 2024 16:41
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