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(cmd): use dot to specify current dir when using --outdir flag #557

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

luigidematteis
Copy link

@luigidematteis luigidematteis commented Nov 20, 2024

Summary

This PR solves the issue #508.

In addition to the --outdir flag for the apply command, the same behavior has been implement also for the other commands that use the same flag, which are: delete cluster, diff, connect openvpn and dump template.

Testing

  • Check if the furyctl apply --outdir . works without issues:
furyctl apply --outdir . --debug
DEBU logging to: .furyctl/furyctl.1732099416-96165.log 
DEBU Set outDir to currentDir: /*/fury/minikube-fury-cluster
INFO Downloading distribution...                  
INFO Compatibility patches applied for v1.29.4    
INFO Validating configuration file...             
INFO Downloading dependencies...                  
DEBU Cleaning vendor folder                       
INFO Validating dependencies...                   
INFO Running preflight checks                     
... 
Apply script completed.
INFO Kubernetes Fury Distribution installed successfully 
INFO Applying plugins...                          
DEBU config path = /var/folders/gx/dy2nbmh950df3qzkh3_dfrsc0000gn/T/furyctl-plugins-3451290346/config.yaml 
INFO Skipping plugins phase as spec.plugins is not defined 
INFO Saving furyctl configuration file in the cluster... 
secret/furyctl-config serverside-applied
INFO Saving distribution configuration file in the cluster... 
secret/furyctl-kfd serverside-applied
  • Check dump template command
INFO Downloading distribution...                  
INFO Compatibility patches applied for v1.29.4    
INFO Validating configuration file...             
INFO Generating distribution manifests...         
INFO Distribution manifests generated successfully 
  • Check if dump template --workdir x --outdir . works as expected (the .furyctl is created in the current directory, not in the workdir)
# $(PWD) == folder_x
furyctl dump template --workdir /folder_y -o .
...
ls /folder_x
.furyctl
  • Check diff command
furyctl diff --workdir /tmp -o .
INFO Downloading distribution...                  
INFO Compatibility patches applied for v1.29.4    
INFO No differences found from previous cluster configuration 
  • Check connect openvpn command

Work in progress...

  • Check delete cluster command
furyctl delete cluster --workdir $(PWD) --outdir .
INFO Downloading distribution...                  
INFO Compatibility patches applied for v1.29.4    
INFO Validating configuration file...             
INFO Downloading dependencies...                  
INFO Validating dependencies...                   
WARNING: You are about to delete a cluster. This action is irreversible.
Are you sure you want to continue? Only 'yes' will be accepted to confirm.
yes
INFO Running preflight checks                     
INFO Checking that the cluster is reachable...    
INFO Preflight checks completed successfully      
INFO Deleting Kubernetes Fury Distribution...     
INFO Checking that the cluster is reachable...    
INFO Checking storage classes...                  
INFO Deleting kubernetes resources...             
...

Breaking changes:

There are not breaking changes.

Copy link
Contributor

@g-iannelli g-iannelli left a comment

Choose a reason for hiding this comment

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

I don't like the change in default behavior for --output-dir. Using the user's home directory is safer than using the current directory. No one can guarantee that someone won't run a command like furyctl dump template -c /dir1/dir2/dir3/furcytl.yaml and write it to the current directory (which is unknown) instead of using the user's home.

IMHO the user's home should be the default and the check for the --output-dir == . should be added in addition.

@g-iannelli
Copy link
Contributor

g-iannelli commented Nov 20, 2024

Moreover the fix doen't work in case of --workdir is specified

pwd && go run main.go dump template -D -w /tmp -o .
~/Workspace/github.com/luigidematteis/furyctl
DEBU logging to: .furyctl/furyctl.1732113133-33825.log
DEBU Changed working directory to /tmp
DEBU Getting Home Directory Path...
INFO Downloading distribution...
INFO Compatibility patches applied for v1.29.4
INFO Validating configuration file...
INFO Generating distribution manifests...
DEBU config path = /var/folders/k9/3lv0pm253td7mqnyhqfryprw0000gn/T/furyctl-dist-2246877313/config.yaml
DEBU output directory = /private/tmp/distribution
INFO Distribution manifests generated successfully

With the old $(pwd) workaround it works fine

pwd && go run main.go dump template -D -w /tmp -o $(pwd)
~/Workspace/github.com/luigidematteis/furyctl
DEBU logging to: ~/Workspace/github.com/luigidematteis/furyctl/.furyctl/furyctl.1732113242-9295.log
DEBU Changed working directory to /tmp
DEBU Getting Home Directory Path...
INFO Downloading distribution...
INFO Compatibility patches applied for v1.29.4
INFO Validating configuration file...
INFO Generating distribution manifests...
DEBU config path = /var/folders/k9/3lv0pm253td7mqnyhqfryprw0000gn/T/furyctl-dist-3600317983/config.yaml
DEBU output directory = ~/Workspace/github.com/luigidematteis/furyctl/distribution
INFO Distribution manifests generated successfully

@luigidematteis
Copy link
Author

I don't like the change in default behavior for --output-dir. Using the user's home directory is safer than using the current directory. No one can guarantee that someone won't run a command like furyctl dump template -c /dir1/dir2/dir3/furcytl.yaml and write it to the current directory (which is unknown) instead of using the user's home.

IMHO the user's home should be the default and the check for the --output-dir == . should be added in addition.

Hi @g-iannelli, I got your point, but this change was only for the apply command, not for dump template. In any case, I agree that it would be better to just add it as an additional check.

Regarding the dump template command, I just noticed that we already had a check for setting the currentDir, but it was not working because we was checking if value of outDir was empty instead of .

I made a fix for that as well.

@luigidematteis luigidematteis changed the title fix(cmd): use dot to specify current dir when using --outdir flag on apply cmd fix(cmd): use dot to specify current dir when using --outdir flag Nov 20, 2024
@luigidematteis luigidematteis changed the title fix(cmd): use dot to specify current dir when using --outdir flag feat(cmd): use dot to specify current dir when using --outdir flag Nov 20, 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.

2 participants