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

fix: ensure we always have an ID #70

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

fernandezcuesta
Copy link
Contributor

@fernandezcuesta fernandezcuesta commented Oct 27, 2024

Description of your changes

id field in tfstate must be either filled in or omitted. This PR attempts to fix it by setting an intermediate value (PENDING) not to fall into the raised bug.

Fixes #66

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Locally with valid credentials.

@fernandezcuesta
Copy link
Contributor Author

fernandezcuesta commented Oct 27, 2024

Not sure if it is possible to omit the id from the tfstate by instructing upjet, but when we get the following we hit the bug as per #66:

The solution is either omit the "id" attribute or put any value on it (current workaround)

{
  "version": 4,
  "terraform_version": "1.5.5",
  "serial": 1,
  "lineage": "8a917ef2-2690-4360-9d29-0bf2a4dfe8b7",
  "outputs": null,
  "resources": [
    {
      "mode": "managed",
      "type": "pagerduty_team",
      "name": "test-team",
      "provider": "provider[\"registry.terraform.io/PagerDuty/pagerduty\"]",
      "instances": [
        {
          "schema_version": 0,
          "attributes": {
            "description": "Test team",
            "id": "",   <------------------- THIS SEEMS TO BE THE OFFENDER
            "name": "Test Team"
          }
        }
      ]
    }
  ]
}

Signed-off-by: Jesús Fernández <[email protected]>
Signed-off-by: Jesús Fernández <[email protected]>
Signed-off-by: Jesús Fernández <[email protected]>
@fernandezcuesta
Copy link
Contributor Author

Looks to me like an error in the upstream terraform provider, which doesn't like having an empty id. Didn't find the way to instruct upjet not to set it in the tfstate

@fnicolelli-ls
Copy link

Hello team,
i've tested this locally and it is working, i can deploy pagerduty Teams without any issues.

@fernandezcuesta
Copy link
Contributor Author

fernandezcuesta commented Nov 11, 2024

@haarchri regarding the "fix" here, what I did so far:

  • Check upstream tf provider, but its model seems OK. Whenever the id field is present but empty, it fails as stated here
  • Check if we can omit the id field in the tfstate, but couldn't find how to instruct upjet to do so
  • Set a fake id during the plan phase, which is basically ignored but fixes the wrong behaviour.
  • Bump provider version and get rid of the restrictions on the discovered resources, add new resource (Jira integration)
  • Fix the IDs of some resources available as of 0.10.0, which were not correct (not as simple as the ID field)
  • Fix some missing selectors for those new resources
  • Update examples from examples-generated
  • Bump dependencies

Do you know if there's any way to omit the id field in tfstate? If not, I guess the solution here

Signed-off-by: Jesús Fernández <[email protected]>
@fernandezcuesta fernandezcuesta force-pushed the fix/team-resource branch 7 times, most recently from b03bf70 to b5b1da3 Compare November 11, 2024 18:56
Signed-off-by: Jesús Fernández <[email protected]>
Signed-off-by: Jesús Fernández <[email protected]>
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.

Provider could not decode JSON response
2 participants