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.

Sign up for GitHub

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

Jump to bottom

fix: skip config load for cmds that don't require it #396

Merged
merged 1 commit into from
Aug 29, 2022
Merged

fix: skip config load for cmds that don't require it #396

merged 1 commit into from
Aug 29, 2022

Conversation

sudo-suhas
Copy link
Contributor

@sudo-suhas sudo-suhas commented Aug 16, 2022 •
edited
Loading

  • Load config only for 'lint' and 'run' commands. This would ensure that
    there are no warnings logs related to config file not being found for
    commands that do not require it.
  • In case the config file is not present, print the error but proceed
    with defaults.
  • Remove metrics/instrumentation for lint command since it does not
    provide any value.

Closes #394

@sudo-suhas

This comment was marked as outdated.

Sign in to view
@sudo-suhas sudo-suhas force-pushed the config-as-required branch 2 times, most recently from 29b1a64 to 2ffe865 Compare August 16, 2022 13:28
@ravisuhag
Copy link
Member

@sudo-suhas Checkout this PR as well. raystack/frontier#155 We should follow a similar approach on this. cc @mabdh @StewartJingga

mabdh
mabdh reviewed Aug 22, 2022
View reviewed changes
cmd/root.go Outdated Show resolved Hide resolved
@sudo-suhas
Copy link
Contributor Author

@sudo-suhas Checkout this PR as well. raystack/frontier#155 We should follow a similar approach on this.

@ravisuhag could you elaborate on this a bit? Are you suggesting that we should use github.com/odpf/salt/cmdx for loading the config?

@ravisuhag
Copy link
Member

@sudo-suhas Checkout this PR as well. odpf/shield#155 We should follow a similar approach on this.

@ravisuhag could you elaborate on this a bit? Are you suggesting that we should use github.com/odpf/salt/cmdx for loading the config?

I think that as well, and also handling of missing config approach.

@sudo-suhas sudo-suhas changed the title fix: rm config load err msgs for cmds that don't require it fix: skip config load for cmds that don't require it Aug 29, 2022
@sudo-suhas
Copy link
Contributor Author

@ravisuhag @StewartJingga have done the changes as per our discussion and also updated the PR desc.

@ravisuhag ravisuhag self-requested a review August 29, 2022 06:27
@sudo-suhas
fix: skip config load for cmds that don't require it
86fef42 
- Load config only for 'lint' and 'run' commands. This would ensure that
  there are no warnings logs related to config file not being found for
  commands that do not require it.
- In case the config file is not present, print the error but proceed
  with defaults.
- Remove metrics/instrumentation for lint command since it does not
  provide any value.
@sudo-suhas
Copy link
Contributor Author

In the sync review/QA for changes, @StewartJingga recommended to still print error for run and lint commands if config file is not present. Have done this change.

@sudo-suhas sudo-suhas merged commit 093d8a6 into raystack:main Aug 29, 2022
@sudo-suhas sudo-suhas deleted the config-as-required branch August 29, 2022 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ravisuhag ravisuhag ravisuhag approved these changes

@mabdh mabdh mabdh approved these changes

@StewartJingga StewartJingga Awaiting requested review from StewartJingga

@rahmatrhd rahmatrhd Awaiting requested review from rahmatrhd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: config not found message on all commands that do not require it
3 participants
@sudo-suhas @ravisuhag @mabdh