Import functionality #225

skudriavtsev · 2022-07-25T13:42:41Z

No description provided.

…plementing 'import' functionality'

surskitt · 2022-08-02T08:42:55Z

Is this a new PR for this functionality? Is it ready to be merged and released?

achernevskii · 2022-08-03T16:57:03Z

infoblox/provider.go

+func newInternalResourceIdFromString(id string) *internalResourceId {
+	newUUID, err := uuid.Parse(id)
+	if err != nil {
+		return nil


Shouldn't we add a log message here, or even panic?

How the user will know if the id is not parsed, and why it's not parsed.

achernevskii · 2022-08-03T17:00:56Z

infoblox/provider.go

+	return true
+}
+
+func generateAltId(internalId *internalResourceId, ref string) string {


What's AltId? What this function will generate?

This AltId is an ID which is alternative to Terraform resource's ID. Sometimes we cannot use NIOS object's reference as a Terraform resource's ID because the reference may get changed.

achernevskii · 2022-08-03T17:18:13Z

infoblox/resource_infoblox_ip_allocation.go

+			if found {
+				stringVal, ok := rawValue.(string)
+				// TODO: do we need this after changing internalId's implementation?
+				if ok && isValidInternalId(stringVal) {


We should make the check for uuid validity inline instead of creating a new isValidInternalId function, which only purpose is to be used in this if statement.

Suggested change

if ok && isValidInternalId(stringVal) {

if _, err := uuid.Parse(stringVal); ok && err == nil {

As of now, this is just for one place in the code, but there is no guarantee that this will not be changed in the future for some reason. That is why I decided to abstract from particular format here. Let it be defined in the code which determines alternative/internal ID's logic.

infoblox/provider.go

achernevskii · 2022-08-03T17:37:08Z

infoblox/provider.go

+		internalId.String(), altIdSeparator, ref)
+}
+
+// valid = true:


Suggested change

// valid = true:

// valid == true if:

achernevskii · 2022-08-03T17:44:54Z

infoblox/resource_infoblox_a_record.go

@@ -15,6 +16,10 @@ func resourceARecord() *schema.Resource {
 		Update: resourceARecordUpdate,
 		Delete: resourceARecordDelete,

+		Importer: &schema.ResourceImporter{
+			State: stateImporter,


FYI State field is deprecated:
https://github.com/hashicorp/terraform-plugin-sdk/blob/v2.4.3/helper/schema/resource_importer.go#L20

Yes, but I planned to clean up all the deprecated fields/types later. Probably it is possible to do it now easily. I will try.

achernevskii · 2022-08-03T17:52:50Z

infoblox/resource_infoblox_ip_allocation.go

 	objMgr := ibclient.NewObjectManager(m.(ibclient.IBConnector), "Terraform", "")
-	hostRec, err = objMgr.SearchHostRecordByAltId(internalId, ref, eaNameForInternalId)
+	hostRec, err = objMgr.SearchHostRecordByAltId(internalId.String(), ref, eaNameForInternalId)


Why this SearchHostRecordByAltId function is defined in the infoblox-go-client?

AltId property is terraform-provider specific, so this func should be defined in this repo.

It is not Terraform specific. Actually, it is a combined searching, by a NIOS object's reference at 1st, then (if not found) by an EA with the given name and value.

I guess, this issue is not in the scope of this review, so I will create a separate ticket in the infoblox-go-client repo, and resolve this thread.

Created infobloxopen/infoblox-go-client#179

achernevskii · 2022-08-03T19:20:31Z

infoblox/provider.go

+//   - ... and separated by the delimiter
+//   - ... and the 1st one is a valid internal ID
+//   - ... and the 2nd one is not empty
+func getAltIdFields(altId string) (internalId *internalResourceId, ref string, valid bool) {


Why don't you return the error value instead of valid bool value?

This will simplify the functions logic down the stack. Each time you're using the valid argument, you format the error in the caller function anyway (here, here, and here).

Instead of formatting error in the caller function each time, just format it in getAltIdFields, and return instead of valid return argument.

Probably you are right. I do not remember the exact reason. I will try to use the canonical way, and if there will be a real reason why to choose the opposite, I will document it with a comment.

achernevskii · 2022-08-09T17:23:30Z

infoblox/provider.go

@@ -21,36 +22,89 @@ const (
 	altIdSeparator      = "|"
 )

-func generateInternalId() string {
-	return uuid.NewString()
+// Internal ID represents an immutable ID during resource's lifecycle.


For godoc it's better to start type comment with the type name:

Suggested change

// Internal ID represents an immutable ID during resource's lifecycle.

// internalResourceId represents an immutable ID during resource's lifecycle.

achernevskii · 2022-08-09T17:24:30Z

infoblox/provider.go

-	if len(idParts) != 2 {
-		err = fmt.Errorf("invalid internal ID for host record: '%s'", altId)
-		return
+// Returns a pointer to parsed internal resource ID, nil otherwise.


Suggested change

// Returns a pointer to parsed internal resource ID, nil otherwise.

// newInternalResourceIdFromString returns a pointer to parsed internal resource ID, nil otherwise.

infoblox/provider.go

Co-authored-by: Aleksei Chernevskii <[email protected]>

surskitt · 2022-08-10T13:33:51Z

This is great to see.

I have to ask now, when should we expect to see it in a release?

Would you recommend against us building the provider from the current master just to perform the resource importing we've needed? I'm intending on just using it for the import then downgrading back to the stable release.

SudoSpartanDan · 2022-08-10T20:51:46Z

@surskitt Yes! Agreed. @achernevskii @skudriavtsev Ideas on a release date?

collenkhanyile · 2022-08-11T07:47:52Z

Thank you guys for all the good work. Release date atleast?

ranjishmp · 2022-08-11T07:51:44Z

Release date yet to confirm as QA team is yet to validate. Also we are planning to include Data Source support for all the possible resources along with this release. This is under development now

adding the import functionality to the resources

47d1b44

skudriavtsev added the draft label Jul 25, 2022

1) moving to the latest version of infoblox-go-client; 2) actually im…

c93df1d

…plementing 'import' functionality'

skudriavtsev removed the draft label Jul 27, 2022

achernevskii suggested changes Aug 3, 2022

View reviewed changes

ranjishmp mentioned this pull request Aug 4, 2022

added import functionality to the resources #218

Closed

achernevskii mentioned this pull request Aug 4, 2022

Make SearchHostRecordByAltId method more generic infobloxopen/infoblox-go-client#179

Open

skudriavtsev added 2 commits August 8, 2022 01:12

updating 'vendor' tree

5b3fccb

addressing some review comments

9a890e2

achernevskii approved these changes Aug 9, 2022

View reviewed changes

minor improvement

495d969

Co-authored-by: Aleksei Chernevskii <[email protected]>

skudriavtsev merged commit 57c847c into infobloxopen:master Aug 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import functionality #225

Import functionality #225

skudriavtsev commented Jul 25, 2022 •

edited

Loading

surskitt commented Aug 2, 2022

achernevskii Aug 3, 2022

skudriavtsev Aug 4, 2022

achernevskii Aug 3, 2022

skudriavtsev Aug 4, 2022

achernevskii Aug 3, 2022

skudriavtsev Aug 4, 2022

achernevskii Aug 3, 2022

achernevskii Aug 3, 2022

skudriavtsev Aug 4, 2022

achernevskii Aug 3, 2022

skudriavtsev Aug 4, 2022

achernevskii Aug 4, 2022

achernevskii Aug 4, 2022

achernevskii Aug 3, 2022

skudriavtsev Aug 4, 2022

achernevskii Aug 9, 2022

achernevskii Aug 9, 2022

surskitt commented Aug 10, 2022

SudoSpartanDan commented Aug 10, 2022

collenkhanyile commented Aug 11, 2022

ranjishmp commented Aug 11, 2022

	if ok && isValidInternalId(stringVal) {
	if _, err := uuid.Parse(stringVal); ok && err == nil {

	// Internal ID represents an immutable ID during resource's lifecycle.
	// internalResourceId represents an immutable ID during resource's lifecycle.

	// Returns a pointer to parsed internal resource ID, nil otherwise.
	// newInternalResourceIdFromString returns a pointer to parsed internal resource ID, nil otherwise.

Import functionality #225

Import functionality #225

Conversation

skudriavtsev commented Jul 25, 2022 • edited Loading

surskitt commented Aug 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surskitt commented Aug 10, 2022

SudoSpartanDan commented Aug 10, 2022

collenkhanyile commented Aug 11, 2022

ranjishmp commented Aug 11, 2022

skudriavtsev commented Jul 25, 2022 •

edited

Loading