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

*: rename packet to equinixmetal #277

Merged
merged 3 commits into from
Jan 21, 2022
Merged

Conversation

tormath1
Copy link
Contributor

@tormath1 tormath1 commented Jan 17, 2022

  • marked --packet-* options as deprecated (still work but not visible)
  • added packet aliase for ore commands

Signed-off-by: Mathieu Tortuyaux [email protected]


Example with current commands:

+ sudo ./bin/kola run --packet-api-key=1234 --packet-facility=xx --packet-image-url=https://bucket.release.flatcar-linux.net/flatcar-jenkins/stable/boards/amd64-usr/3033.2.0/flatcar_production_packet_image.bin.bz2 --packet-installer-image-base-url=https://bucket.release.flatcar-linux.net/flatcar-jenkins/stable/boards/amd64-usr/3033.2.0 --packet-project=1234 ... kubeadm.v1.23.0.flannel.base
Flag --packet-api-key has been deprecated, packet options are deprecated, please update to equinixmetal options
Flag --packet-facility has been deprecated, packet options are deprecated, please update to equinixmetal options
Flag --packet-image-url has been deprecated, packet options are deprecated, please update to equinixmetal options
Flag --packet-installer-image-base-url has been deprecated, packet options are deprecated, please update to equinixmetal options
Flag --packet-project has been deprecated, packet options are deprecated, please update to equinixmetal options
Flag --packet-storage-url has been deprecated, packet options are deprecated, please update to equinixmetal options
2022-01-17T14:04:29Z cli: Started logging at level DEBUG
packet platform is deprecated, updating to equinixmetal
2022/01/17 15:04:29 [DEBUG] POST https://api.packet.net/ssh-keys
=== RUN   kubeadm.v1.23.0.flannel.base

and for ore command:

./bin/ore equinixmetal --help
EquinixMetal machine utilities

Usage:
  ore equinixmetal [command]

Aliases:
  equinixmetal, packet
...
  • Changelog entries added in the respective changelog/ directory (user-facing change, bug fix, security fix, update)

Closes: flatcar/Flatcar#561

@tormath1 tormath1 self-assigned this Jan 17, 2022
@tormath1 tormath1 marked this pull request as ready for review January 17, 2022 14:22
@tormath1 tormath1 requested a review from a team January 17, 2022 14:23
Copy link
Member

@jepio jepio left a comment

Choose a reason for hiding this comment

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

Like it, thanks for keeping the old flags supported for the transition.

CHANGELOG.md Outdated
@@ -8,6 +8,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- plume: Add new AWS regions, af-south-1, ap-southeast-3, eu-south-1 ([#274](https://github.com/flatcar-linux/mantle/pull/274))
- kubernetes test for release 1.23.0 ([#275](https://github.com/flatcar-linux/mantle/pull/275))

### Changed
- removed `packet` occurences in favor of `equinixmetal` ([#277](https://github.com/flatcar-linux/mantle/pull/277))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- removed `packet` occurences in favor of `equinixmetal` ([#277](https://github.com/flatcar-linux/mantle/pull/277))
- removed `packet` occurrences in favor of `equinixmetal` ([#277](https://github.com/flatcar-linux/mantle/pull/277))

cmdCreateDevice.Flags().StringVar(&options.ImageURL, "image-url", "", "image base URL (default board-dependent, e.g. \"https://alpha.release.flatcar-linux.net/amd64-usr/current/flatcar_production_packet_image.bin.bz2\")")
cmdCreateDevice.Flags().StringVar(&options.InstallerImageKernelURL, "installer-image-kernel-url", "", "EquinixMetal installer image kernel URL, (default installer-image-base-url/flatcar_production_pxe.vmlinuz)")
cmdCreateDevice.Flags().StringVar(&options.InstallerImageCpioURL, "installer-image-cpio-url", "", "EquinixMetal installer image cpio URL, (default installer-image-base-url/flatcar_production_pxe_image.cpio.gz)")
cmdCreateDevice.Flags().StringVar(&options.ImageURL, "image-url", "", "image base URL (default board-dependent, e.g. \"https://alpha.release.flatcar-linux.net/amd64-usr/current/flatcar_production_equinixmetal_image.bin.bz2\")")
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this link is 404 as we still produce "packet" images, not "equinixmetal" ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I tried to preserve the URL links but I missed this one. It's just an example value but let's keep things correct. :)

@tormath1
Copy link
Contributor Author

@krnowak your suggestions are committed. I also took the opportunity to split into two commits: sed + backward compatibility to ease the revert of the last one later.

@@ -22,26 +22,26 @@ import (
"path/filepath"
)

const PacketConfigPath = ".config/packet.json"
const EquinixMetalConfigPath = ".config/equinixmetal.json"
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this won't break if someone had a config named packet.json. This value is used as a default value for --packet-config-file (now --equinixmetal-config-file). I haven't seen --packet-config-file being used in our Jenkins setup, so not sure we should care.

It is also used in ore equinixmetal --config-file too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a doubt for this one, at first I started to support both - but I dropped the idea since it's a default config... Not sure about what we should do.

Copy link
Member

Choose a reason for hiding this comment

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

Eh, if something breaks then we can revert this one line.

@@ -0,0 +1,70 @@
// Copyright 2017 CoreOS, Inc.
Copy link
Member

@sayanchowdhury sayanchowdhury Jan 18, 2022

Choose a reason for hiding this comment

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

Let's update the license here with the #263 changes.

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, since it's a brand new file, header has been updated to:

// Copyright The Mantle Authors
// SPDX-License-Identifier: Apache-2.0

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't say it's a new file - just a rename of an old one…

Copy link
Member

Choose a reason for hiding this comment

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

The line Copyright 2017 CoreOS, Inc. must stay

kola/tests/flannel/flannel.go Outdated Show resolved Hide resolved
kola/tests/flannel/flannel.go Outdated Show resolved Hide resolved
kola/tests/flannel/flannel.go Outdated Show resolved Hide resolved
@tormath1
Copy link
Contributor Author

@sayanchowdhury thanks for your suggestion - comments have been addressed.

@@ -0,0 +1,59 @@
// Copyright The Mantle Authors
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Copyright The Mantle Authors
// Copyright The Mantle Authors
// Copyright 2017 CoreOS, Inc.

Copy link
Member

Choose a reason for hiding this comment

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

Original copyright statements should not be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouh yes. Thanks !

Signed-off-by: Mathieu Tortuyaux <[email protected]>
* marked `--packet-*` options as deprecated (still work but not visible)
* added `packet` aliase for `ore` commands

Signed-off-by: Mathieu Tortuyaux <[email protected]>
Signed-off-by: Mathieu Tortuyaux <[email protected]>
@tormath1 tormath1 merged commit 2e8c1c8 into flatcar-master Jan 21, 2022
@tormath1 tormath1 deleted the tormath1/bye-packet branch January 21, 2022 10:25
tormath1 added a commit to flatcar/scripts that referenced this pull request Jan 26, 2022
Follow up from: flatcar/mantle#277

Signed-off-by: Mathieu Tortuyaux <[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.

mantle: rename packet to equinix-metal
5 participants