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

feat: migrate to tf-sdk2 #102

Merged
merged 59 commits into from
Mar 18, 2021
Merged

feat: migrate to tf-sdk2 #102

merged 59 commits into from
Mar 18, 2021

Conversation

mavogel
Copy link
Contributor

@mavogel mavogel commented Dec 27, 2020

Steps

see https://www.terraform.io/docs/extend/guides/v2-upgrade-guide.html

Notes

@mavogel mavogel changed the title feat migrate to tf-sdk2 feat: migrate to tf-sdk2 Dec 27, 2020
@mavogel mavogel mentioned this pull request Dec 27, 2020
@mavogel mavogel added this to the v2.10.0 milestone Dec 27, 2020
@mavogel mavogel linked an issue Dec 27, 2020 that may be closed by this pull request
@innovate-invent
Copy link
Contributor

Have a look at some of the changes in #23. You will need to actually use the new context param for it to be effective.

@mavogel
Copy link
Contributor Author

mavogel commented Dec 28, 2020

Thanks for pointing out! I made the rough changes yesterday and now I'll go for the details

@mavogel mavogel self-assigned this Dec 29, 2020
@mavogel mavogel force-pushed the feat-migrate-tf-sdk2 branch 2 times, most recently from eecea6e to 3e82110 Compare January 6, 2021 18:56
@mavogel mavogel removed this from the v2.10.0 milestone Jan 7, 2021
Type: schema.TypeMap,
Type: schema.TypeList,
Copy link
Collaborator

Choose a reason for hiding this comment

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

State Migration should be implemented.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out :) So for all TypeMap to TypeList we need those migrators right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • tested locally with unit tests and
  • as described in the contributing section (I wonder how we can automate those tests)
terraform {
  required_providers {
    docker = {
      source  = "kreuzwerker/docker"
      #version = "2.8.0"
      version = "9.9.9"
    }
  }
}

provider "docker" {
}

resource "docker_image" "foo" {
  name         = "nginx:latest"
  keep_locally = true
}

resource "docker_service" "foo" {
  name = "foo"
  task_spec {
    container_spec {
      image             = docker_image.foo.latest
      stop_grace_period = "10s"
    }
    # v2.8.0
    #restart_policy = { 
    # v9.9.9
    restart_policy {
      condition    = "on-failure"
      delay        = "3s"
      max_attempts = 4
      window       = "10s"
    }

  }

  endpoint_spec {
    ports {
      target_port    = "80"
      published_port = "8080"
    }
  }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great work!
Would you split files per schema version like docker_container resource?

  • resource_docker_service.go
  • resource_docker_service_v1.go
  • resource_docker_service_v0.go

I did the same in thing hashicorp/terraform-provider-docker#272 .

Copy link
Collaborator

@suzuki-shunsuke suzuki-shunsuke left a comment

Choose a reason for hiding this comment

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

Is it required to move docker package to internal/provider for SDK migration?

@suzuki-shunsuke
Copy link
Collaborator

Great work!

IMHO, we should do one thing in one pull request.
In case of this pull request, we should do only SDK migration in this pull request, and refactoring and bug fixes which are unrelated to the SDK migration should be done in the other pull requests.

I don't think we should split this pull request forcibly because it is hard,
but I wrote this comment for the future work.

@mavogel
Copy link
Contributor Author

mavogel commented Mar 15, 2021

Is it required to move docker package to internal/provider for SDK migration?

I wanted to be consistent with the latest scaffolder

@suzuki-shunsuke
Copy link
Collaborator

I wanted to be consistent with the latest scaffolder

I got it. Looks Good.

@mavogel
Copy link
Contributor Author

mavogel commented Mar 17, 2021

Feel free to leave a final review @suzuki-shunsuke :)

@suzuki-shunsuke suzuki-shunsuke self-requested a review March 17, 2021 09:18
Copy link
Collaborator

@suzuki-shunsuke suzuki-shunsuke left a comment

Choose a reason for hiding this comment

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

🚀

@suzuki-shunsuke
Copy link
Collaborator

I can't merge this pull request.
I think we should fix the branch protection rule although I don't have the permission.

image

@mavogel mavogel added this to the v3.0.0 milestone Mar 18, 2021
@mavogel mavogel merged commit ad3e56d into master Mar 18, 2021
@mavogel mavogel deleted the feat-migrate-tf-sdk2 branch March 18, 2021 07:30
@mavogel mavogel modified the milestones: v3.0.0, v2.12.0 May 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: update to terraform-plugin-sdk2
3 participants