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

fix: create folder for the config #806

Merged
merged 4 commits into from
Jul 7, 2021
Merged

fix: create folder for the config #806

merged 4 commits into from
Jul 7, 2021

Conversation

wtrocki
Copy link
Collaborator

@wtrocki wtrocki commented Jul 7, 2021

No description provided.

Copy link
Contributor

@craicoverflow craicoverflow left a comment

Choose a reason for hiding this comment

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

I had issues when setting the custom config location:

export RHOASCONFIG=~/.rhoascustom
❯ ./rhoas whoami
unable to read config file: read /home/ephelan/.rhoascustom: is a directory

I think if RHOASCONFIG is set, and the directory does not exist we should not try to create the directory for them. Only when the system defaulr dir does not exist should we create it.

cmd/rhoas/main.go Outdated Show resolved Hide resolved
cmd/rhoas/main.go Outdated Show resolved Hide resolved
@wtrocki
Copy link
Collaborator Author

wtrocki commented Jul 7, 2021

Actually handling for RHOASCONFIG is done in other place.

@wtrocki
Copy link
Collaborator Author

wtrocki commented Jul 7, 2021

This is much cleaner now

@wtrocki wtrocki requested a review from craicoverflow July 7, 2021 11:10
@wtrocki
Copy link
Collaborator Author

wtrocki commented Jul 7, 2021

The only thing that is see right now is that you still going to get default folder created even if custom location is set.
My take is that we should always create that folder.

@craicoverflow
Copy link
Contributor

The only thing that is see right now is that you still going to get default folder created even if custom location is set.
My take is that we should always create that folder.

We could also check if the env is set, and only create the folder if it is not set. But I'm happpy with how it is right now.

cmd/rhoas/main.go Outdated Show resolved Hide resolved
cmd/rhoas/main.go Outdated Show resolved Hide resolved
@wtrocki
Copy link
Collaborator Author

wtrocki commented Jul 7, 2021

@craicoverflow I tried to add HasCustomLocation: func() bool to config but I seem to have problem with mock generator right now. Do not want to use this env variable in two places

@wtrocki wtrocki requested a review from craicoverflow July 7, 2021 11:29
@wtrocki
Copy link
Collaborator Author

wtrocki commented Jul 7, 2021

I think that would be good right now

@wtrocki wtrocki merged commit e1f5747 into main Jul 7, 2021
@wtrocki wtrocki deleted the config-fix branch July 7, 2021 11:32
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