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

Add IBMi Software License field to instance data source and resource #5082

Merged
merged 31 commits into from
Feb 28, 2024

Conversation

ismirlia
Copy link
Collaborator

This PR adds the software license fields to instance data source and resource. These fields can be updated. I have added a new acceptance test.

--- PASS: TestAccIBMPIInstanceIBMiLicense (1869.89s)
PASS

@ismirlia ismirlia added the service/Power Systems Issues related to Power Systems label Jan 29, 2024
Copy link
Collaborator

@michaelkad michaelkad left a comment

Choose a reason for hiding this comment

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

Can you update the documentation? Since those will only be in ibmi.

website/docs/d/pi_instance.html.markdown Outdated Show resolved Hide resolved
website/docs/r/pi_instance.html.markdown Outdated Show resolved Hide resolved
@ismirlia ismirlia requested a review from michaelkad February 5, 2024 20:03
@michaelkad
Copy link
Collaborator

@ismirlia could you fix the ibm/service/power/ibm_pi_constants.go conflict?

@ismirlia
Copy link
Collaborator Author

ismirlia commented Feb 8, 2024

@yussufsh @hkantare are there any more comments? Can we get this merged?

@hkantare
Copy link
Collaborator

fix conflicts

ibm/service/power/data_source_ibm_pi_instance.go Outdated Show resolved Hide resolved
ibm/service/power/resource_ibm_pi_network.go Outdated Show resolved Hide resolved
ibm/service/power/data_source_ibm_pi_instance.go Outdated Show resolved Hide resolved
@ismirlia ismirlia requested a review from yussufsh February 16, 2024 18:10
@ismirlia
Copy link
Collaborator Author

ok I think I misunderstood what you were saying earlier. let me fix it

Comment on lines +368 to +369
Optional: false,
Required: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kind of contradicting.
Here is the doc:

	// Required indicates whether the practitioner must enter a value in the
	// configuration for this attribute. Required cannot be used with Computed
	// Default, DefaultFunc, DiffSuppressFunc, DiffSuppressOnRefresh,
	// InputDefault, Optional, or StateFunc. At least one of Required,
	// Optional, Optional and Computed, or Computed must be enabled.
	Required [bool](https://pkg.go.dev/builtin#bool)

	// Optional indicates whether the practitioner can choose to not enter
	// a value in the configuration for this attribute. Optional cannot be used
	// with Required.
	Optional [bool](https://pkg.go.dev/builtin#bool)

	// Computed indicates whether the provider may return its own value for
	// this attribute or not. Computed cannot be used with Required. If
	// Required and Optional are both false, the attribute will be considered
	// "read only" for the practitioner, with only the provider able to set
	// its value.
	Computed [bool](https://pkg.go.dev/builtin#bool)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@yussufsh I believe there is no contradiction, since the attribute is set by the provider as mention above if both false fir Required and Optional

If Required and Optional are both false, the attribute will be considered "read only" for the practitioner, with only the provider able to set its value.

Copy link
Collaborator

@yussufsh yussufsh Feb 23, 2024

Choose a reason for hiding this comment

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

Computed cannot be used with Required.
Just Computed = true was enough.

Just common logic:
Required = true (cannot be Optional)
Optional = true (cannot be Required)

Comment on lines +818 to +820
if ibmrdsUsers < 0 {
return diag.Errorf("request with IBMi Rational Dev Studio property requires IBMi Rational Dev Studio number of users")
}
Copy link
Collaborator

@yussufsh yussufsh Feb 21, 2024

Choose a reason for hiding this comment

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

Not a requirement but we should be using ValidateFunc (ValidateDiagFunc)
https://developer.hashicorp.com/terraform/plugin/sdkv2/schemas/schema-behaviors#validatefunc

Copy link
Collaborator Author

@ismirlia ismirlia Feb 21, 2024

Choose a reason for hiding this comment

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

That's probably a bit more than I want to change for this PR. Probably mark it as an enhancement.

Comment on lines +1483 to +1485
if ibmrdsUsers.(int) < 0 {
return nil, fmt.Errorf("request with IBMi Rational Dev Studio property requires IBMi Rational Dev Studio number of users")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ismirlia ismirlia requested a review from yussufsh February 21, 2024 16:18
Copy link
Collaborator

@yussufsh yussufsh left a comment

Choose a reason for hiding this comment

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

lgtm

@ismirlia
Copy link
Collaborator Author

@hkantare Please merge this if there are no more comments.

@hkantare hkantare merged commit 3c07f0b into IBM-Cloud:master Feb 28, 2024
1 check passed
ismirlia added a commit to powervs-ibm/terraform-provider-ibm that referenced this pull request Mar 4, 2024
…BM-Cloud#5082)

* Initial ibmi

* ibmi initial, and sync with master

* sync go.mod and go.sum

* sync go.mod and go.sum

* Update instance data source with IBMi licenses

* Update instance resource with IBMi licenses

* Add instance resource IBMi license acceptance test

* Add active check for instance update

* Fix updating licenses

* Update IBMi terraform documentation

* Add wait software licenses update function

* Update IBMi license test

* Update instance data source

* Update instance update license markdown

* Remove duplicate markdown definitions

* Refactor ibmi license code

* Refactor ibmi license code

* Update IBMi documentation

* Fix duplicate constants

* Fix formatting

* Fix formatting

* Fix typo from merge conflict

* Refactor variable names

* Fix ibmi documentation

* Separate ibmi attribute in data source form ibmi argument in resource

---------

Co-authored-by: michaelkad <michaelkadiayi@gmail.com>
Co-authored-by: michael kad <michaelkad>
@michaelkad michaelkad deleted the add_ibm_soft_license branch May 28, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service/Power Systems Issues related to Power Systems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants