Simplify the configuration loading #437
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: petrkotas The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #437 +/- ##
==========================================
- Coverage 32.08% 30.48% -1.60%
==========================================
Files 35 36 +1
Lines 2425 2529 +104
==========================================
- Hits 778 771 -7
- Misses 1587 1699 +112
+ Partials 60 59 -1
🚀 New features to boost your workflow:
|
petrkotas
force-pushed
the
simplify-ocm-config
branch
2 times, most recently
from
f661634 to
f2f64c8
Compare
|
/retest |
petrkotas
force-pushed
the
simplify-ocm-config
branch
3 times, most recently
from
676689d to
b6d8bbd
Compare
petrkotas
force-pushed
the
simplify-ocm-config
branch
4 times, most recently
from
1341444 to
537e812
Compare
|
Looks good and generally makes sense to me. Good stuff! |
petrkotas
force-pushed
the
simplify-ocm-config
branch
from
537e812 to
042af60
Compare
openshift-merge-robot
added
the
needs-rebase
petrkotas
force-pushed
the
simplify-ocm-config
branch
2 times, most recently
from
99d38c9 to
5de9384
Compare
openshift-merge-robot
removed
the
needs-rebase
petrkotas
force-pushed
the
simplify-ocm-config
branch
3 times, most recently
from
582f9b1 to
d65ed9a
Compare
openshift-merge-robot
added
the
needs-rebase
|
/retest |
1 similar comment
|
/retest |
petrkotas
force-pushed
the
simplify-ocm-config
branch
from
d65ed9a to
16ddf2c
Compare
openshift-merge-robot
removed
the
needs-rebase
Moves the options to flags. Also moves the env variables to the main command, so there are no sideeffects when running the commands. Signed-off-by: Petr Kotas <pkotas@redhat.com>
petrkotas
force-pushed
the
simplify-ocm-config
branch
from
16ddf2c to
36bc03c
Compare
|
@petrkotas: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| PreRunE: func(cmd *cobra.Command, _ []string) error { | ||
| var err error | ||
| var c config.Config | ||
|
|
||
| c, err = config.BuildConfig(cmd) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to build config: %w", err) | ||
| } | ||
|
|
||
| // Initialize the logger here because it depends on user input | ||
| logging.RawLogger = logging.InitLogger(c.LogLevel) | ||
| logging.RawLogger = logging.WithPipelineName(logging.RawLogger, c.PipelineName) | ||
|
|
||
| // configure the RunE command using a wrapper to | ||
| // append the configuration into the command | ||
| cmd.RunE = func(cmd *cobra.Command, args []string) error { | ||
| if err := run(c, cmd, args); err != nil { | ||
| logging.Error(err) | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| }, | ||
| RunE: func(_ *cobra.Command, _ []string) error { | ||
| // DO NOT REMOVE | ||
| // this ensures RunE work as expected | ||
| // RunE is set in the PreRunE function | ||
| // this works because the RunE is always called after PreRunE | ||
| return nil | ||
| }, |
There was a problem hiding this comment.
Is there a specific reason we need this vs just loading the config & initialize the logger in in run()? It seems like an unnecessary hack to use RunE, maybe I'm missing something.
| var err error | ||
|
|
||
| if c.PrometheusPushGateway == "" { | ||
| logging.Warn("metrics disabled, set env 'CAD_PROMETHEUS_PUSHGATEWAY' to push metrics") | ||
| } | ||
|
|
||
| err = metrics.Push(c.PrometheusPushGateway) |
There was a problem hiding this comment.
We don't need the loose var err error here, this would help with readability.
| var err error | |
| if c.PrometheusPushGateway == "" { | |
| logging.Warn("metrics disabled, set env 'CAD_PROMETHEUS_PUSHGATEWAY' to push metrics") | |
| } | |
| err = metrics.Push(c.PrometheusPushGateway) | |
| if c.PrometheusPushGateway == "" { | |
| logging.Warn("metrics disabled, set env 'CAD_PROMETHEUS_PUSHGATEWAY' to push metrics") | |
| } | |
| err := metrics.Push(c.PrometheusPushGateway) |
See google golang styleguide on error handling: https://google.github.io/styleguide/go/guide#maintainability
| } | ||
|
|
||
| // early init of logger for logs before clusterID is known |
There was a problem hiding this comment.
Is this a leftover? I don't see the relation to the code around it.
| // early init of logger for logs before clusterID is known |
| logging.Warn("metrics disabled, set env 'CAD_PROMETHEUS_PUSHGATEWAY' to push metrics") | ||
| } | ||
|
|
||
| err = metrics.Push(c.PrometheusPushGateway) |
There was a problem hiding this comment.
Would there be any metrics yet at this point in the code? The old logic pushed metrics after they were set (post run logic), now we push before we run anything IIUC.
There was a problem hiding this comment.
I think another issue is that we first check if the PushGateway is empty, but the proceed to push metrics to it, even if it is empty.
| if c.PagerDutyToken == "" { | ||
| return fmt.Errorf("missing PagerDuty token") | ||
| } | ||
| if c.PagerDutySilentPolicy == "" { | ||
| return fmt.Errorf("missing PagerDuty silent policy") | ||
| } |
There was a problem hiding this comment.
What's the intent behind checking this again here?
We already checked that we loaded something while building the config. If we want to check for completeness/format, this should ideally also go to the config building to allow us to exit as early as possible once we detect we're missing something required.
| ocmConfigFile = file | ||
| } | ||
|
|
||
| func HasConfigFile() bool { |
There was a problem hiding this comment.
Is this function used? If not, can we remove it? I couldn't find any references.
| return nil, fmt.Errorf("one of the required envvars in the list '(CAD_SILENT_POLICY CAD_PD_TOKEN)' is missing") | ||
| } | ||
|
|
||
| func GetPDClient(webhookPayload []byte, cadPD string, cadSilentPolicy string) (*SdkClient, error) { |
| // Initialize the logger | ||
| logging.RawLogger = logging.InitLogger("info") |
There was a problem hiding this comment.
Two comments:
- Thought: Hmm, this is a bit of an unwanted side effect of the ptr, but I suppose we'll get rid of this in a later refactor.
- Nitpick: this is a useless comment & we don't comment it in other files either. We can remove it for consistency.
| // Initialize the logger | |
| logging.RawLogger = logging.InitLogger("info") | |
| logging.RawLogger = logging.InitLogger("info") |
| if c.BackplaneURL == "" { | ||
| return fmt.Errorf("missing backplane URL") | ||
| } | ||
| if c.BackplaneProxyURL == "" { | ||
| logging.Warn("missing backplane proxy URL, using default") | ||
| } | ||
| if c.BackplaneInitialARN == "" { | ||
| return fmt.Errorf("missing backplane initial ARN") | ||
| } |
There was a problem hiding this comment.
It's a bit late to check these. Would we not have caught this when loading the config at the start?
What's the default for BackplaneProxyURL?
| } | ||
|
|
||
| client := SdkClient{} | ||
| var err error |
There was a problem hiding this comment.
Why are we introducing this specific line?
|
The latest review is mostly style/chore type changes. The one major issue I see is that the metrics are likely broken by moving the pushgateway push() to the start of the investigation run. |
The code simplifies the ocm config loading. It also moves the env variables right to the start,
so it is obvious when they are used.
JIRA: https://issues.redhat.com/browse/SREP-71