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.

Sign up for GitHub

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

Jump to bottom

Computeenvironment and jobqueue #1542

Merged
merged 5 commits into from
Oct 29, 2024
Merged

Computeenvironment and jobqueue #1542

merged 5 commits into from
Oct 29, 2024

Conversation

sachinmalanki
Copy link
Contributor

@sachinmalanki sachinmalanki commented Oct 25, 2024 •
edited by turkenf
Loading

Description of your changes

Creating new AWS Batch Resources - ComputeEnvironment and JobQueue
Continuing from - #1489

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

This code has been tested locally and with uptest in the below comments.

@sachinmalanki sachinmalanki mentioned this pull request Oct 25, 2024
Closed
3 tasks
@sachinmalanki
Copy link
Contributor Author

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

3 similar comments
@turkenf
Copy link
Collaborator

turkenf commented Oct 25, 2024

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

@sachinmalanki
Copy link
Contributor Author

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Oct 25, 2024

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

@sachinmalanki
Copy link
Contributor Author

The Uptest fails because of this error.
logger.go:42: 13:57:17 | case/0-apply | - lastTransitionTime: "2024-10-25T13:56:19Z" logger.go:42: 13:57:17 | case/0-apply | message: 'update failed: async update failed: failed to set observation: v1beta1.ComputeEnvironmentObservation.ComputeResources: logger.go:42: 13:57:17 | case/0-apply | readObjectStart: expect { or n, but found [, error found in #10 byte of ...|sources":[{"allocati|..., logger.go:42: 13:57:17 | case/0-apply | bigger context ...|,"id":"sample","region":null,"compute_resources":[{"allocation_strategy":"","desired_vcpus":0,"max_v|...' logger.go:42: 13:57:17 | case/0-apply | reason: ReconcileError logger.go:42: 13:57:17 | case/0-apply | status: "False" logger.go:42: 13:57:17 | case/0-apply | type: Synced logger.go:42: 13:57:17 | case/0-apply | - lastTransitionTime: "2024-10-25T13:42:44Z" logger.go:42: 13:57:17 | case/0-apply | reason: Available logger.go:42: 13:57:17 | case/0-apply | status: "True" logger.go:42: 13:57:17 | case/0-apply | type: Ready logger.go:42: 13:57:17 | case/0-apply | - lastTransitionTime: "2024-10-25T13:56:19Z" logger.go:42: 13:57:17 | case/0-apply | message: 'async update failed: failed to set observation: v1beta1.ComputeEnvironmentObservation.ComputeResources: logger.go:42: 13:57:17 | case/0-apply | readObjectStart: expect { or n, but found [, error found in #10 byte of ...|sources":[{"allocati|..., logger.go:42: 13:57:17 | case/0-apply | bigger context ...|,"id":"sample","region":null,"compute_resources":[{"allocation_strategy":"","desired_vcpus":0,"max_v|...' logger.go:42: 13:57:17 | case/0-apply | reason: AsyncUpdateFailure logger.go:42: 13:57:17 | case/0-apply | status: "False" logger.go:42: 13:57:17 | case/0-apply | type: LastAsyncOperation

Any idea on how to fix this? I can see that compute resource expects object but is provided an array which is likely the reason for this error.

turkenf
turkenf reviewed Oct 28, 2024
View reviewed changes
config/batch/config.go Outdated
Comment on lines 9 to 21
func Configure(p *config.Provider) {
p.AddResourceConfigurator("aws_batch_compute_environment", func(r *config.Resource) {
r.References = config.References{
"compute_resources.instance_role": config.Reference{
TerraformName: "aws_iam_instance_profile",
},
"compute_resources.placement_group": config.Reference{
TerraformName: "aws_placement_group",
},
"service_role": config.Reference{
TerraformName: "aws_iam_role",
},
"compute_resources.subnets": config.Reference{
TerraformName: "aws_subnet",
},
"compute_resources.security_group_ids": config.Reference{
TerraformName: "aws_security_group",
},
}
r.UseAsync = true
})

p.AddResourceConfigurator("aws_batch_job_queue", func(r *config.Resource) {
r.References = config.References{
"compute_environment_order.compute_environment": config.Reference{
TerraformName: "aws_batch_compute_environment",
Extractor: common.PathARNExtractor,
},
}
})
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tested these two resources in my local, and except for the automatically defined references, defining the above seems sufficient. Could you please remove the above and add only the following configurations?

func Configure(p *config.Provider) {
	p.AddResourceConfigurator("aws_batch_compute_environment", func(r *config.Resource) {
		r.References["compute_resources.subnets"] = config.Reference{
			TerraformName: "aws_subnet",
		}
		r.References["compute_resources.security_group_ids"] = config.Reference{
			TerraformName: "aws_security_group",
		}
	})
}

examples/batch/v1beta1/jobqueue.yaml Outdated
Comment on lines 19 to 20
providerConfigRef:
name: aws-provider-config-test
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use the default provider config in uptest; could you please remove the above?

examples/batch/v1beta1/jobqueue.yaml Outdated
Comment on lines 54 to 55
providerConfigRef:
name: aws-provider-config-test
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use the default provider config in uptest; could you please remove the above?

@turkenf
Copy link
Collaborator

turkenf commented Oct 28, 2024

Could you please also remove the batch group from externalnamenottested.go

Thank you 🙏

@sachinmalanki
feat: add compute env and jon queue
e5a4733 
Signed-off-by: sachinmalanki <sachin.chini@gmail.com>
@turkenf
Copy link
Collaborator

turkenf commented Oct 28, 2024

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Oct 28, 2024

/test-examples="examples/iot/v1beta1/topicruledestination.yaml"

@sachinmalanki
fix: update file permissions
ce668b3 
Signed-off-by: sachinmalanki <sachin.chini@gmail.com>
@turkenf turkenf force-pushed the computeenv-jobqueue branch from ce668b3 to 33d589a Compare October 28, 2024 17:18
@turkenf
Copy link
Collaborator

turkenf commented Oct 28, 2024

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

@turkenf turkenf force-pushed the computeenv-jobqueue branch from 33d589a to e5a4733 Compare October 28, 2024 17:34
@turkenf
Copy link
Collaborator

turkenf commented Oct 28, 2024

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

@sachinmalanki
fix: update example roles
0d0a65c 
Signed-off-by: sachinmalanki <sachin.chini@gmail.com>
@turkenf
Copy link
Collaborator

turkenf commented Oct 28, 2024

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Oct 28, 2024

/test-examples="examples/batch/v1beta1/jobqueue.yaml"

@sachinmalanki
fix: keep pre hooks only in role
e75e510 
Signed-off-by: sachinmalanki <sachin.chini@gmail.com>
@turkenf
Copy link
Collaborator

turkenf commented Oct 28, 2024 •
edited
Loading

/test-examples="examples/batch/v1beta1/computeenvironment.yaml"

Uptest run: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/11562496508

@turkenf
Copy link
Collaborator

turkenf commented Oct 28, 2024 •
edited
Loading

/test-examples="examples/batch/v1beta1/jobqueue.yaml"

Uptest run: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/11562654341

turkenf
turkenf reviewed Oct 28, 2024
View reviewed changes
Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

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

Many thanks for your effort and quick actions @sachinmalanki. The tests were finally passed successfully 🎉

Finally, a few cosmetic corrections would be nice before moving forward. I left a few comments for your consideration.

examples/batch/v1beta1/jobqueue.yaml Outdated Show resolved Hide resolved
examples/batch/v1beta1/testhooks/delete-computeenvironment.sh Outdated Show resolved Hide resolved
examples/batch/v1beta1/computeenvironment.yaml Outdated Show resolved Hide resolved
@sachinmalanki
fix: make PR ready
574f83f 
Signed-off-by: sachinmalanki <sachin.chini@gmail.com>
@sachinmalanki
Copy link
Contributor Author

Thanks @turkenf for your help. I have made the small changes suggested

@jeanduplessis jeanduplessis merged commit 0016fd6 into crossplane-contrib:main Oct 29, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turkenf turkenf turkenf left review comments

@jeanduplessis jeanduplessis jeanduplessis approved these changes

@ulucinar ulucinar Awaiting requested review from ulucinar ulucinar is a code owner

@sergenyalcin sergenyalcin Awaiting requested review from sergenyalcin sergenyalcin is a code owner

@mbbush mbbush Awaiting requested review from mbbush mbbush is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sachinmalanki @turkenf @jeanduplessis