Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Add MQ broker and configuration resources #193

Merged
merged 1 commit into from
Apr 29, 2022

Conversation

ezgidemirel
Copy link
Collaborator

@ezgidemirel ezgidemirel commented Apr 28, 2022

Signed-off-by: ezgidemirel ezgidemirel91@gmail.com

Description of your changes

This PR adds MQ configuration and broker resources.

Fixes #192

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

I created/deleted an MQ broker and a configuration using example manifests.

Copy link
Collaborator

@turkenh turkenh left a comment

Choose a reason for hiding this comment

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

@ezgidemirel could you also fill how this code has been tested section?

func Configure(p *config.Provider) {
p.AddResourceConfigurator("aws_mq_broker", func(r *config.Resource) {
r.Version = common.VersionV1Alpha2
// Due to a terrajet limitation, we cannot use "metedata.name" field as the name of the resource
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you put the link for that issue here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @ezgidemirel,
This issue resembles crossplane-contrib/provider-mongodbatlas#1, although my understanding is that broker ID does not entail any piece of information from spec.forProvider. But still, if I'm not mistaken, crossplane-contrib/provider-mongodbatlas#1 is a generalized version of what you have encountered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, looks like it is. I tried to apply some workarounds to omit name fields, but couldn't be successful.

metadata:
name: example
spec:
forProvider:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I note here on why we need the brokerName here instead of reusing metadata.name would be helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

metadata:
name: example
spec:
forProvider:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, please add a note.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Signed-off-by: ezgidemirel <ezgidemirel91@gmail.com>
@ezgidemirel
Copy link
Collaborator Author

Thanks a lot @turkenh for your review!

@ezgidemirel ezgidemirel merged commit 211912f into crossplane-contrib:main Apr 29, 2022
@ezgidemirel ezgidemirel deleted the add-mq-broker branch April 29, 2022 16:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support MQ resources
3 participants