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.

Sign up for GitHub

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

Jump to bottom

Changed username and urls to computed to fix the idempotency #231

Merged
merged 2 commits into from
Jul 26, 2023
Merged

Changed username and urls to computed to fix the idempotency #231

merged 2 commits into from
Jul 26, 2023

Conversation

anvitha-jain
Copy link
Collaborator

solves #224

@anvitha-jain anvitha-jain linked an issue Jul 20, 2023 that may be closed by this pull request
Cannot add site with mso_site #224
Closed
@anvitha-jain anvitha-jain self-assigned this Jul 20, 2023
shrsr
shrsr requested changes Jul 20, 2023
View reviewed changes
Copy link
Collaborator

@shrsr shrsr left a comment

Choose a reason for hiding this comment

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

Just the comment above--

@anvitha-jain anvitha-jain reopened this Jul 20, 2023
@anvitha-jain anvitha-jain requested a review from shrsr July 20, 2023 19:10
shrsr
shrsr requested changes Jul 20, 2023
View reviewed changes
mso/resource_mso_site.go Outdated Show resolved Hide resolved
shrsr
shrsr previously approved these changes Jul 20, 2023
View reviewed changes
Copy link
Collaborator

@shrsr shrsr left a comment

Choose a reason for hiding this comment

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

LGTM

anvitha-jain
anvitha-jain commented Jul 24, 2023
View reviewed changes
Copy link
Collaborator Author

@anvitha-jain anvitha-jain left a comment

Choose a reason for hiding this comment

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

Had too revert by removing computed: true for location argument since it affects idempotency.

@anvitha-jain anvitha-jain requested a review from shrsr July 24, 2023 17:53
akinross
akinross previously approved these changes Jul 24, 2023
View reviewed changes
Copy link
Collaborator

@akinross akinross left a comment

Choose a reason for hiding this comment

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

LGTM

shrsr
shrsr requested changes Jul 25, 2023
View reviewed changes
Copy link
Collaborator

@shrsr shrsr left a comment

Choose a reason for hiding this comment

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

Adding Computed : True to location is affecting the idempotent behaviour because your Terraform configuration file does not have a value for location. When you try to re-apply after the first apply your state which has now has a value for location is compared with your configuration which finds the location empty.

Let's discuss with Lionel if we want the omit this.

@anvitha-jain anvitha-jain requested a review from shrsr July 25, 2023 18:10
shrsr
shrsr previously approved these changes Jul 25, 2023
View reviewed changes
Copy link
Collaborator

@shrsr shrsr left a comment

Choose a reason for hiding this comment

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

LGTM

akinross
akinross previously approved these changes Jul 26, 2023
View reviewed changes
Copy link
Collaborator

@akinross akinross left a comment

Choose a reason for hiding this comment

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

LGTM

@anvitha-jain
Changed username and urls to computed to fix the idempotency
a191d0c
@anvitha-jain
[minor_changes] Fixing adding site, after manually changing it to unm…
8f3be2c 
…anaged. Also, fix errorForObjectNotFound function in utils to error out when new "error" payload is found.
@anvitha-jain anvitha-jain dismissed stale reviews from akinross and shrsr via 8f3be2c July 26, 2023 20:12
lhercot
lhercot approved these changes Jul 26, 2023
View reviewed changes
Copy link
Member

@lhercot lhercot left a comment

Choose a reason for hiding this comment

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

LGTM

@lhercot lhercot merged commit 3f9fdb8 into CiscoDevNet:master Jul 26, 2023
@anvitha-jain anvitha-jain deleted the site_vals branch August 28, 2023 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhercot lhercot lhercot approved these changes

@shrsr shrsr shrsr left review comments

@akinross akinross akinross left review comments

@sajagana sajagana Awaiting requested review from sajagana

@abrahammughal abrahammughal Awaiting requested review from abrahammughal

Assignees

@anvitha-jain anvitha-jain

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot add site with mso_site
4 participants
@anvitha-jain @lhercot @shrsr @akinross