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

feat: add vultr container registry resource #445

Merged
merged 7 commits into from
Jun 3, 2024

Conversation

im6h
Copy link
Contributor

@im6h im6h commented Dec 16, 2023

Description

  • Based on Vultr API, add resource container registry and data container registry
  • Add docs and examples to use the new resource

Checklist:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you linted your code locally prior to submission?
  • Have you successfully ran tests with your changes locally?

@im6h im6h changed the title feat: add vultr container registry resource [WIP] feat: add vultr container registry resource Dec 16, 2023
@im6h im6h changed the title [WIP] feat: add vultr container registry resource feat: add vultr container registry resource Dec 16, 2023
@im6h
Copy link
Contributor Author

im6h commented Dec 16, 2023

I fixed golint and added resources to terrafrom code
@optik-aper Would you like to check it again? Thanks

@Stealthmate
Copy link

I'd really like to see this merged!
I'm in the process of writing TF manifests for all my stuff on Vultr, and container registry is one of the first things I need to have in order to bootstrap everything else.

I'm no Go expert, but I can hack on this and try to resolve the conflicts if needed.

@optik-aper
Copy link
Member

Sorry for the delay! I'll test this out and merge it this afternoon.

Copy link
Member

@optik-aper optik-aper left a comment

Choose a reason for hiding this comment

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

This works for the most part. The tests aren't valid based on the requirements of the API, so that should be fixed. Some of the data schema isn't completely accurate either. Ideally this would include some documentation in the website/docs/r directory, but that can be added later.

A data_source for container registry would be nice to have, but not crucial to the basic functionality here. Another nice thing to have would be validation on the name field to ensure that the limited set of characters isn't allowed through. But neither of those are blockers for this PR.

vultr/resource_vultr_container_registry.go Outdated Show resolved Hide resolved
vultr/resource_vultr_container_registry.go Outdated Show resolved Hide resolved
vultr/resource_vultr_container_registry.go Outdated Show resolved Hide resolved
vultr/resource_vultr_container_registry.go Outdated Show resolved Hide resolved
@im6h
Copy link
Contributor Author

im6h commented Feb 4, 2024

I'd really like to see this merged! I'm in the process of writing TF manifests for all my stuff on Vultr, and container registry is one of the first things I need to have in order to bootstrap everything else.

I'm no Go expert, but I can hack on this and try to resolve the conflicts if needed.

Sure. I think we can make PR better before merging into the main

@F21
Copy link
Contributor

F21 commented Jun 3, 2024

Is there anything I can do to help with this one? Pretty keen to see this merged.

@optik-aper
Copy link
Member

Is there anything I can do to help with this one? Pretty keen to see this merged.

I just tested things out and it's working. There are a few things I'd like to see cleaned up, and the name validation currently won't account for names with non-alphanumeric characters, but I will address that in a separate PR. I'll get a new release out today or tomorrow.

Copy link
Member

@optik-aper optik-aper left a comment

Choose a reason for hiding this comment

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

Approved with some updates to follow

@optik-aper optik-aper merged commit 337a974 into vultr:master Jun 3, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants