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: add Darwin supporting back #106

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Conversation

Yu-Jack
Copy link
Contributor

@Yu-Jack Yu-Jack commented Sep 27, 2024

Related Issues

Description

Since go-common-libs did a breaking change:

From:

func (nsexec *Executor) Execute(binary string, args []string, timeout time.Duration) (string, error) {
	return nsexec.executor.Execute(nil, types.NsBinary, nsexec.prepareCommandArgs(binary, args), timeout)
}

To:

func (nsexec *Executor) Execute(envs []string, binary string, args []string, timeout time.Duration) (string, error) {
	return nsexec.executor.Execute(nil, types.NsBinary, nsexec.prepareCommandArgs(binary, args, envs), timeout)
}

So, we need to bump following dependencies:

  • Bump Harvester to v1.3.1
  • Replace go-common-libs

Test Plan

  1. Generate binary
CGO_ENABLED=0 go build -mod=vendor -o bin/terraform-provider-harvester .
mkdir -p ~/.terraform.d/plugins/registry.terraform.io/harvester/harvester/0.0.0-dev/darwin_arm64
cp bin/terraform-provider-harvester ~/.terraform.d/plugins/registry.terraform.io/harvester/harvester/0.0.0-dev/darwin_arm64/terraform-provider-harvester_v0.0.0-dev
  1. Prepare a provider.tf
terraform {
  required_providers {
    harvester = {
      source = "harvester/harvester"
      version = "0.0.0-dev" 
    }
  }
}

provider "harvester" {
    kubeconfig = "your harvester kube config"
}
  1. Use following main.tf to test
resource "harvester_image" "ubuntu20" {
   name      = "ubuntu-focal"
   namespace = "default"

   display_name = "ubuntu-focal"
   source_type  = "download"
   url          = "https://cloud-images.ubuntu.com/focal/current/focal-server-cloudimg-amd64.img"
 }

resource "harvester_cloudinit_secret" "cloud-config-ubuntu20" {
  name      = "cloud-config-ubuntu20"
  namespace = "default"

  user_data    = <<-EOF
    #cloud-config
    password: test
    chpasswd:
      expire: false
    ssh_pwauth: true
    package_update: true
    packages:
      - qemu-guest-agent
    runcmd:
      - - systemctl
        - enable
        - '--now'
        - qemu-guest-agent
    EOF
  network_data = ""
}

resource "harvester_virtualmachine" "ubuntu20" {
  name                 = "ubuntu20"
  namespace            = "default"
  restart_after_update = true

  description = "test ubuntu20 raw image"
  tags = {
    ssh-user = "ubuntu"
  }

  cpu    = 1
  memory = "2Gi"

  run_strategy    = "RerunOnFailure"
  hostname        = "ubuntu20"
  reserved_memory = "100Mi"
  machine_type    = "q35"

  network_interface {
    name           = "nic-1"
    wait_for_lease = true
  }

  disk {
    name       = "rootdisk"
    type       = "disk"
    size       = "10Gi"
    bus        = "virtio"
    boot_order = 1

    image       = harvester_image.ubuntu20.id
    auto_delete = true
  }

  cloudinit {
    user_data_secret_name    = harvester_cloudinit_secret.cloud-config-ubuntu20.name
    network_data_secret_name = harvester_cloudinit_secret.cloud-config-ubuntu20.name
  }
}

@Yu-Jack Yu-Jack self-assigned this Sep 27, 2024
Copy link
Member

@bk201 bk201 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bk201 bk201 requested a review from m-ildefons September 30, 2024 02:46
Copy link
Member

@FrankYang0529 FrankYang0529 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you. Test with goreleaser:

  1. Add a temporary tag.
git tag v0.6.6
  1. Remove dist folder.
rm -rf dist
  1. Build.
> goreleaser build
  • loading environment variables
  • getting and validating git state
    • git state                                      commit=ec81b3b84818d61fd663d8ee52bf934e7be5934a branch=PR-106 current_tag=v0.6.6 previous_tag=v0.6.5 dirty=false
  • parsing tag
  • setting defaults
  • running before hooks
    • running                                        hook=go mod tidy
  • ensuring distribution directory
  • setting up metadata
  • writing release metadata
  • loading go mod information
  • build prerequisites
  • building binaries
    • building                                       binary=dist/terraform-provider-harvester_darwin_arm64/terraform-provider-harvester_v0.6.6
    • building                                       binary=dist/terraform-provider-harvester_darwin_amd64_v1/terraform-provider-harvester_v0.6.6
    • building                                       binary=dist/terraform-provider-harvester_linux_amd64_v1/terraform-provider-harvester_v0.6.6
    • building                                       binary=dist/terraform-provider-harvester_linux_arm64/terraform-provider-harvester_v0.6.6
    • took: 54s
  • writing artifacts metadata
  • build succeeded after 54s
  • thanks for using goreleaser!

@FrankYang0529
Copy link
Member

Hi @m-ildefons, could you help to review this? Thank you.

Copy link
Contributor

@m-ildefons m-ildefons left a comment

Choose a reason for hiding this comment

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

LGTM.

I have no Darwin system to test this change on, but it looks sane to me.

I've looked at why we even depend on importing longhorn in the first place and it looks to me like the only place where we utilize the Longhorn library is in the storage class resource schema. On top of that, the only thing from the Longhorn library we use is the constant LonghornDriverName as the default value for the provider for the storage class resource. IMO, we should hard-code this as a local constant and remove the dependency on the Longhorn library completely. What do you think?

@Yu-Jack
Copy link
Contributor Author

Yu-Jack commented Oct 21, 2024

LGTM.

I have no Darwin system to test this change on, but it looks sane to me.

I've looked at why we even depend on importing longhorn in the first place and it looks to me like the only place where we utilize the Longhorn library is in the storage class resource schema. On top of that, the only thing from the Longhorn library we use is the constant LonghornDriverName as the default value for the provider for the storage class resource. IMO, we should hard-code this as a local constant and remove the dependency on the Longhorn library completely. What do you think?

@m-ildefons I agree with it. If we all agree with this one, I could remove it in this PR until we really need it. Otherwise, too much unnecessary dependencies can be inconvenient and confusing. What do you think? @bk201 @FrankYang0529

@bk201
Copy link
Member

bk201 commented Oct 21, 2024

Sound good it the constant is never expected to change.

@Yu-Jack Yu-Jack force-pushed the HARV/6655 branch 2 times, most recently from cc7e912 to ec81b3b Compare October 22, 2024 02:34
@Yu-Jack
Copy link
Contributor Author

Yu-Jack commented Oct 22, 2024

@m-ildefons I just realized because Harvester v1.3.1 still uses LH as dependency, so removing LH dependency doesn't have too much effect. It still needs replacement. So, I'll just merge this PR. cc @bk201 @FrankYang0529

@Yu-Jack Yu-Jack merged commit e9d53b0 into harvester:master Oct 22, 2024
5 checks passed
@m-ildefons m-ildefons mentioned this pull request Oct 22, 2024
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.

4 participants