Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Reset switch in .flux.yaml #2638

Merged
merged 5 commits into from
Jan 23, 2020
Merged

Reset switch in .flux.yaml #2638

merged 5 commits into from
Jan 23, 2020

Conversation

squaremo
Copy link
Member

@squaremo squaremo commented Nov 27, 2019

Briefly: let people reset the processing rules in .flux.yaml, so you can have a .flux.yaml at the top of the git repo, but switch that off for a particular --git-path.

(At present this is branched from another PR, because I will need to write a bit of documentation about this new thing, and better to do so in revised docs)

@squaremo squaremo force-pushed the act-normal-mode branch 2 times, most recently from 97aa249 to e81c127 Compare January 21, 2020 10:17
@squaremo squaremo marked this pull request as ready for review January 21, 2020 10:17
@squaremo squaremo requested a review from 2opremio January 21, 2020 10:58
command:
type: object
required: ['command']
version: { const: 1 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we bump the version?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? No-one has used this yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

But we are changing the file format

Copy link
Member Author

Choose a reason for hiding this comment

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

Versioning here is to prevent misunderstandings about what the file means; in particular, to make sure that you aren't giving fluxd an instruction it doesn't know how to carry out.

Can you think of a potential mistake that would be avoided by bumping the version here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not absolutely against it, but it does mean writing code that would discriminate between the two versions (assuming we'd want to not invalidate everyone's config immediately.)

Copy link
Contributor

@2opremio 2opremio Jan 22, 2020

Choose a reason for hiding this comment

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

Can you think of a potential mistake that would be avoided by bumping the version here?

No, not really.

I am not absolutely against it, but it does mean writing code that would discriminate between the two versions (assuming we'd want to not invalidate everyone's config immediately.)

Yeah, that's a pain.

I think that, technically, we should bump the version number but I am not sure it's worth the pain.

Maybe we could use minor version numbers for backward-compatible syntax changes (without needing to handle it differently in the code). Or maybe we can just leave it at version 1 .

@2opremio
Copy link
Contributor

Can you extend the .flux.yaml documentation with the additional syntax?

@squaremo
Copy link
Member Author

Now I'm writing some docs, it occurs to me that this:

version: 1
files: {}

looks like it says "include the following files (none)". The map value {} is just to have a value for the field, and it's a map because maybe we want to add more stuff there later, and otherwise any value would do.

Thinking thinking thinking ...

@squaremo
Copy link
Member Author

Now I'm writing some docs, it occurs to me that this:

version: 1
files: {}

looks like it says "include the following files (none)".

Use another word:

version: 1
plainFilesMode: {}
  • doesn't look like it's supposed to be including or excluding specific files

Use another, constant value:

version: 1
files: true
  • it's obvious that it's a toggle (although what is the meaning of files: false?)
  • adding more configs means a breaking schema change

Of these I think I prefer the first.

@2opremio
Copy link
Contributor

how about something a bit more descriptive

version: 1
scanThisDirForManifestFiles: true

@stefanprodan
Copy link
Member

I like files: {}, it gives us the the possibility to add glob filters later on, something like:

version: 1
files:
 include: "*prod.yaml"
 exclude: "crd*"

@squaremo
Copy link
Member Author

I like files: {}, it gives us the the possibility to add glob filters later on

Yeah I like it for that reason too; it's just that the empty value (for now, the only possible value) gives the wrong impression.

To date there's been some gentle checks on .flux.yaml files, but it's
still easy to (for example) include a patchFile field with a
commandUpdated config, and think that it should work. Putting a full
schema in means mistakes get caught much earlier.
This adds the schema and struct for a `files` field in
`.flux.yaml`. This is one of the alternatives for how to generate
files, and is intended to mean "act as though there were no
.flux.yaml` in effect" (implementation to follow ..).
This adds a special case for a config file that acts as a reset
switch, by having a `files: ` entry instead of `patchUpdated` or
`commandUpdated`.

Since the desired behaviour is to act as though there were _no_ config
file in effect, by far the most direct implementation is to add the
path in question to the collection of raw files paths when a config
file containing the directive is found.
@squaremo
Copy link
Member Author

Replaced files: {} with scanForFiles: {}, which I think is descriptive enough, and avoids the ambiguity. Possibly.

@2opremio 2opremio added this to the 1.18.0 milestone Jan 23, 2020
The required form of a .flux.yaml using files is

    version: 1
    files: {}

which looks an awful lot like indicating that no files should be
processed.

In this commit I've renamed the directive to `scanForFiles`, which is
less likely to cause this particular confusion.
@squaremo squaremo merged commit 1516b06 into master Jan 23, 2020
@squaremo squaremo deleted the act-normal-mode branch January 23, 2020 10:44
@squaremo
Copy link
Member Author

Merge it while it's hot!

Thanks @2opremio and @stefanprodan for reviews and comments.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants