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

move builder and cleanup #1

Merged
merged 6 commits into from
Jun 19, 2024

Conversation

Vicente-Cheng
Copy link
Collaborator

@Vicente-Cheng Vicente-Cheng commented Jun 1, 2024

TODO ITEM

  • move builder to dapper
  • bump golang
  • remove redundant
  • improve the provisioner

GOOD TO HAVE

  • create new GHA
  • update readme

@Vicente-Cheng Vicente-Cheng force-pushed the wip-refine-build branch 2 times, most recently from c5d2062 to 203b500 Compare June 3, 2024 16:51
@Vicente-Cheng Vicente-Cheng marked this pull request as ready for review June 4, 2024 03:29
@Vicente-Cheng Vicente-Cheng requested review from bk201 and tserong June 4, 2024 03:32
@Vicente-Cheng Vicente-Cheng changed the base branch from master to main June 4, 2024 03:46
@Vicente-Cheng Vicente-Cheng force-pushed the wip-refine-build branch 2 times, most recently from 918e0bb to a380585 Compare June 4, 2024 03:55
@bk201
Copy link
Member

bk201 commented Jun 4, 2024

Just did a build locally:

Built harvester/harvester-lvm-csi-plugin:a380585-amd64
Built harvester/harvester-lvm-provisioner:a380585-amd64

The repository name should be rancher. harvester/harvester-lvm-csi-plugin -> rancher/harvester-lvm-csi-plugin.
We need an EIO ticket to setup the repo and secrets.

    - Also, bump golang v1.22
    - fix the complain from complier

Signed-off-by: Vicente Cheng <vicente.cheng@suse.com>
Signed-off-by: Vicente Cheng <vicente.cheng@suse.com>
@Vicente-Cheng
Copy link
Collaborator Author

Copy link

@tserong tserong left a comment

Choose a reason for hiding this comment

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

LGTM in general, just a couple of small things below.

Also, main.go line 82 specifies this:

        driverName        = flag.String("drivername", "lvm.csi.metal-stack.io", "name of the driver")

I know we set this to lvm.driver.harvesterhci.io in deploy/charts/values.yaml,
but should we also change the default in main.go as this is our fork? And, if we do change the default, maybe we don't need to specify it in the chart values?

package/Dockerfile.provisioner Outdated Show resolved Hide resolved
.github/workflows/factory.yml Show resolved Hide resolved
    - Also, update the driver default name

Signed-off-by: Vicente Cheng <vicente.cheng@suse.com>
Signed-off-by: Vicente Cheng <vicente.cheng@suse.com>
    - We sometimes will stuck on the lvcreate due lacking the udev
      operations. But actually lvm can handle the device well.
      So we involve the host-side lvm config and skip the udev_sync and
      udev_rules. Give the lvm handle the whole things.

Signed-off-by: Vicente Cheng <vicente.cheng@suse.com>
    - Also, remove the redundant GHA

Signed-off-by: Vicente Cheng <vicente.cheng@suse.com>
@Vicente-Cheng
Copy link
Collaborator Author

LGTM in general, just a couple of small things below.

Also, main.go line 82 specifies this:

        driverName        = flag.String("drivername", "lvm.csi.metal-stack.io", "name of the driver")

I know we set this to lvm.driver.harvesterhci.io in deploy/charts/values.yaml, but should we also change the default in main.go as this is our fork? And, if we do change the default, maybe we don't need to specify it in the chart values?

Nice catch, driverName updated!

@Vicente-Cheng Vicente-Cheng requested a review from tserong June 19, 2024 06:44
@Vicente-Cheng Vicente-Cheng merged commit 537231d into harvester:main Jun 19, 2024
1 check passed
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.

3 participants