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

Take default support instead of rely on existence of config #211

Merged

Conversation

martinkunc
Copy link
Contributor

While loading config, serialized.ToController was supposing that all values will be explicitly set in serialized config (config/local.yaml file), instead of using default values from support.Controller.
This change will use default values, unless they are overwritten by serializable config, or later overwritten by from secrets.
Before this change, loading logic required the PullReport.Delay to be set, which leads to crash on serialized.ToController error with delay must be a non-negative duration

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2020
Endpoint: s.Endpoint,
ReportEndpoint: s.PullReport.Endpoint,
Impersonate: s.Impersonate,
func (s *Serialized) ToController(cfg *Controller) (*Controller, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to send the cfg attribute? We can always create a new Controller as before or not? Otherwise it looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is called by Load - so the steps are:
Set default to support.Controller (in code)
Try to load config to support.Controller

And unfortunatelly previously loading was resetting whole support.Controller with what was set in config file, so all the defaults had to be set there. That was ignoring what was already set in support.Controller and creating a new one.

Now, there is an option to not to pass existing Controller and create failback to previous behaviour, but we use the way which passes in the existing support.Controller default values set before.
And there is an option to override the defaults from config. For this there are if len(...)>0 blocks below - if it is set in config file, use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh I see. Sorry. I should have read the description.

@rluders
Copy link
Contributor

rluders commented Oct 13, 2020

It seems a really straightforward solution, so just looking at the code it seems OK.

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: martinkunc, rluders

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2020
@openshift-merge-robot openshift-merge-robot merged commit 94bb271 into openshift:master Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants