Skip to content

Conversation

hardillb
Copy link
Contributor

@hardillb hardillb commented Sep 3, 2024

fixes #312

Description

Default options overwrite port number in config file

Related Issue(s)

#312

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

@hardillb hardillb requested a review from Steve-Mcl September 3, 2024 15:20
@hardillb hardillb self-assigned this Sep 3, 2024
@knolleary
Copy link
Member

I don't think this is the approach we want to take as it will mean explicitly set command-line args do not override the settings file.

The core issue is the fact that if no port option is provided on the command line, the code defaults as if 1880 had been set on the command-line - and overrides the file.

I think the order of precedence should be:

  1. Command Line
  2. Settings file
  3. Default value

That will require a bit more to achieve than just reordering the spread operations

@hardillb hardillb marked this pull request as draft September 4, 2024 09:27
@hardillb
Copy link
Contributor Author

hardillb commented Sep 4, 2024

OK,

So I think if we

  • remove the default options from the default options from the cli config
  • add a default options object to the start of the spread
  • swap the order back to cli parse options last in the spread

e.g.

const defaults = {
  port: 1880.
  ...
}

return {
        version,
        ...defaults,
        ...parsedOptions.deviceConfig
        ...options,
    }

1. command line
2. config file
3. defaults
@hardillb hardillb requested review from knolleary and removed request for Steve-Mcl September 4, 2024 09:50
@hardillb hardillb marked this pull request as ready for review September 4, 2024 10:35
@knolleary knolleary merged commit de6212f into main Sep 6, 2024
4 checks passed
@knolleary knolleary deleted the 312-use-port-from-config branch September 6, 2024 10:52
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.

Port option from device.yml Not Being Applied

2 participants