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

Refactor openevec to base actions on top of type and fix multiple configs #906

Merged
merged 5 commits into from
Oct 19, 2023

Conversation

giggsoff
Copy link
Collaborator

To use the same semantic in openevec we should use the same base type.
Also we use novel changers to load config directly to be able to load config directly from openevec.

We want to refactor the logic to use base type for all exported openevec
 actions

Signed-off-by: Petr Fedchenkov <giggsoff@gmail.com>
To use controller config options directly from config we should
implement several functions

Signed-off-by: Petr Fedchenkov <giggsoff@gmail.com>
To use the same semantic in openevec we should use the same base type.
Also we use novel changers to load config directly

Signed-off-by: Petr Fedchenkov <giggsoff@gmail.com>
Let's use simple scenario where we want to create several EVE-OS
instances in different configs

Signed-off-by: Petr Fedchenkov <giggsoff@gmail.com>
In case of delete it would be nice to not show fatal errors

Signed-off-by: Petr Fedchenkov <giggsoff@gmail.com>
@giggsoff giggsoff linked an issue Oct 13, 2023 that may be closed by this pull request
@uncleDecart
Copy link
Collaborator

@giggsoff I see from review that you are removing cfg from parameters and rely on reading config from file which creates unnecessary coupling. That brings back things that I tried to revert with my previous PRs: you don't have single configuration file object, you will have to use ./eden config add before using any eden command.

I believe that this is not a way to go because you cannot simply create golang structure and use it when calling any openEVEC functions and pass it (see here)

@giggsoff
Copy link
Collaborator Author

@giggsoff I see from review that you are removing cfg from parameters and rely on reading config from file which creates unnecessary coupling. That brings back things that I tried to revert with my previous PRs: you don't have single configuration file object, you will have to use ./eden config add before using any eden command.

I believe that this is not a way to go because you cannot simply create golang structure and use it when calling any openEVEC functions and pass it (see here)

Probably it is not so clear, but I try to do exactly what you mention. Instead of dirty magic inside of re-reading config from viper here I fixed and use InitVarsFromConfig which rely on config object you implemented before (EdenSetupArgs).
So, in you example, you will use something like:

cfg := ec.GetDefaultConfig(curPath)
openEVEC = openevec.CreateOpenEVEC(cfg)
...
openEVEC.PodDeploy(appLink, *pc)

@@ -53,7 +53,7 @@ func newPacketRunCmd(cfg *openevec.EdenSetupArgs, packetKey, packetProjectName *
Use: "run",
Short: "run vm in packet",
Run: func(cmd *cobra.Command, args []string) {
err := openevec.PacketRun(*packetKey, *packetProjectName, packetVMName, packetZone, packetMachineType, packetIPXEUrl, cfg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like you don't need cfg in function anymore

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, can you please elaborate, what exactly you want to see here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately, I can't commit to your PR :D What I meant is yetus is complaining on unused variable, we can fix it by

diff --git a/cmd/edenPacket.go b/cmd/edenPacket.go
index 3f0e22e..82c60c1 100644
--- a/cmd/edenPacket.go
+++ b/cmd/edenPacket.go
@@ -16,7 +16,7 @@ func newPacketCmd(configName, verbosity *string) *cobra.Command {
                Short: `Manage VMs in Equinix Metal Platform`,
        }
 
-       packetCmd.AddCommand(newPacketVMCmd(cfg, &cfg.Packet.Key, &packetProjectName))
+       packetCmd.AddCommand(newPacketVMCmd(&cfg.Packet.Key, &packetProjectName))
 
        packetCmd.PersistentFlags().StringVarP(&cfg.Packet.Key, "key", "k", "", "packet key file")
        packetCmd.PersistentFlags().StringVarP(&packetProjectName, "project", "p", defaults.DefaultPacketProjectName, "project name on packet")
@@ -24,7 +24,7 @@ func newPacketCmd(configName, verbosity *string) *cobra.Command {
        return packetCmd
 }
 
-func newPacketVMCmd(cfg *openevec.EdenSetupArgs, packetKey, packetProjectName *string) *cobra.Command {
+func newPacketVMCmd(packetKey, packetProjectName *string) *cobra.Command {
        var packetVMCmd = &cobra.Command{
                Use:   "vm",
                Short: `Manage VMs in packet`,
@@ -34,7 +34,7 @@ func newPacketVMCmd(cfg *openevec.EdenSetupArgs, packetKey, packetProjectName *s
                {
                        Message: "Basic Commands",
                        Commands: []*cobra.Command{
-                               newPacketRunCmd(cfg, packetKey, packetProjectName),
+                               newPacketRunCmd(packetKey, packetProjectName),
                                newPacketDeleteCmd(packetKey, packetProjectName),
                                newPacketGetIPCmd(packetKey, packetProjectName),
                        },
@@ -46,7 +46,7 @@ func newPacketVMCmd(cfg *openevec.EdenSetupArgs, packetKey, packetProjectName *s
        return packetVMCmd
 }

@@ -57,7 +57,7 @@ func newPodPublishCmd(cfg *openevec.EdenSetupArgs) *cobra.Command {
Args: cobra.ExactArgs(1),
Run: func(cmd *cobra.Command, args []string) {
appName := args[0]
if err := openevec.PodPublish(appName, kernelFile, initrdFile, rootFile, formatStr, arch, local, disks, cfg); err != nil {
if err := openEVEC.PodPublish(appName, kernelFile, initrdFile, rootFile, formatStr, arch, local, disks); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like no need for cfg in function as well

@uncleDecart
Copy link
Collaborator

@giggsoff I'll merge it and will follow up with PR to fix Yetus complains about unused variables

@uncleDecart uncleDecart merged commit 44f8608 into lf-edge:master Oct 19, 2023
7 of 10 checks passed
uncleDecart added a commit to uncleDecart/eden that referenced this pull request Oct 19, 2023
For refactoring PR see lf-edge#906

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
@uncleDecart
Copy link
Collaborator

follow-up PR #907

uncleDecart added a commit to uncleDecart/eden that referenced this pull request Oct 19, 2023
For refactoring PR see lf-edge#906

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
uncleDecart added a commit to uncleDecart/eden that referenced this pull request Oct 19, 2023
For refactoring PR see lf-edge#906

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
giggsoff pushed a commit that referenced this pull request Oct 19, 2023
For refactoring PR see #906

Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
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.

Wrap configuration file as structure in class OpenEVEC
2 participants