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

Use terraform sdk v2 and support importing resources #51

Merged
merged 1 commit into from
Dec 29, 2022

Conversation

cristicalin
Copy link
Contributor

@cristicalin cristicalin commented Nov 20, 2022

This PR updates the provider to use the terraform v2 sdk and implements the ability to use terraform import to import pre-created resources from existing phpipam installations.

Fixes: #49

@cristicalin
Copy link
Contributor Author

@lord-kyron could you take a look at this proposed change?

@pavel-z1 pavel-z1 self-assigned this Dec 13, 2022
Copy link
Collaborator

@pavel-z1 pavel-z1 left a comment

Choose a reason for hiding this comment

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

First of all, thank you for the work you have done!

Please delete the directory pkg. The binary files will be added to the release assets.

In some files need to fix indent. Please format go files with this command to fix indent:

gofmt -w plugin/providers/phpipam/*

It would be great to add an example of using terraform import to the README.md file

Comment on lines 57 to 67
id := d.Id()
if len(id) > 0 {
address_id, err := strconv.Atoi(id)
if err != nil {
return err
}
out[0], err = c.GetAddressByID(address_id)
if err != nil {
return err
}
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't get the point of this change. Can you please add a comment why this check is needed?

Comment on lines 47 to 57
id := d.Id()
if len(id) > 0 {
subnet_id, err := strconv.Atoi(id)
if err != nil {
return err
}
out[0], err = c.GetSubnetByID(subnet_id)
if err != nil {
return err
}
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't get the point of this change. Can you please add a comment why this check is needed?

@cristicalin cristicalin force-pushed the add_import_resources branch 2 times, most recently from 0f56b32 to 6aa23b8 Compare December 19, 2022 10:09
@cristicalin
Copy link
Contributor Author

Hi @pavel-z1, thanks for the review, I made the modifications you requested and explained why I needed to modify the import logic to ensure we don't destroy imported resources.

@pavel-z1
Copy link
Collaborator

Hi @cristicalin !
Thank you for updates

Format please these files and I will merge PR

# go fmt ./plugin/providers/phpipam/*
plugin/providers/phpipam/data_source_phpipam_address.go
plugin/providers/phpipam/data_source_phpipam_subnet.go

@cristicalin
Copy link
Contributor Author

@pavel-z1 I go fmt'ed the two files

@pavel-z1 pavel-z1 merged commit 80912e2 into lord-kyron:master Dec 29, 2022
@pavel-z1
Copy link
Collaborator

Thank you @cristicalin !
PR merged

@Celedhrim Celedhrim mentioned this pull request Feb 22, 2023
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.

documentation around resource imports
2 participants