Skip to content

Conversation

@laverya
Copy link
Member

@laverya laverya commented Mar 7, 2019

  • If any changes have been made to /project files, the corresponding /src files have been regenerated (make project-import PROJECT=<project-name>).
  • If any changes need to be deployed, the version has been bumped in package.json.
  • If any changes impact the replicated-lint package, they have been added to CHANGELOG.

@laverya laverya requested review from Rob0h and dexhorthy March 7, 2019 02:30
src/lint.ts Outdated
let root;
try {
root = yaml.safeLoad(this.inYaml);
root = yaml.safeLoadAll(this.inYaml);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I agree with this 100%.

We added some stuff to the CLI to lint multidoc YAML in #163

I think the doc selection should happen outside the core linter method. I don't think it's harder to do it that way, and reduces responsibilities of linter.lint().

Copy link
Member Author

@laverya laverya Mar 7, 2019

Choose a reason for hiding this comment

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

I don't think that 'doc selection' is the only reason to move this to linter.lint(), though. For one, vendor web calls linter.lint() directly and so does not receive the benefits of a wrapper function - and more relevant here, some of the information about the parsed documents may be useful when parsing an individual document.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think "vendor web calls this with multiple docs" is a reason to move additional YAML parsing logic into this method.

What I'm getting at here is: have we considered letting the client be responsible for choosing the doc? It's not a lot to ask of venweb, and this method is already supler huge.

There may be other reasons to do this here, but I don't see a compelling reason to increase the complexity in the library method just to save clients some code (parsing) that isn't the core focus of replicated-lint (linting)

Copy link
Contributor

@dexhorthy dexhorthy Mar 8, 2019

Choose a reason for hiding this comment

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

You could easily expose the doc selection logic as a helper function, to be called by the client to select a doc before calling linter.lint(), in the case that it's needed

Essentially two narrower interfaces instead of one increasingly broad one.

# kind: scheduler-swarm
#this is not valid yaml
{
Copy link
Member Author

Choose a reason for hiding this comment

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

But before anything else, I need to see if there's a way to fix this sort of thing
(by ignoring mesg-yaml-valid from the 2nd document on, for example)

@dexhorthy
Copy link
Contributor

dexhorthy commented Mar 8, 2019

Stepping up a level -- part of the reason we made it easy to write new rules was so that clients that had more context (e.g. scheduler, features enabled, external registry tags) could bake that context into additional rules outside the core ruleset at linting time.

Rather than "add more context/variables onto the linter object so it can do deeper evaluations" why not have the client, e.g. vendor web, inject two or three additional rules based on it's understanding of the world.

In the grand scheme of things, clients should describe policy, and the linter should just enforce it. The fact that the core replicated YAML ruleset lives in this repo feels like a side effect of how things were built/designed on the fly during rules implementation. Ideally they could all move to projects

@dexhorthy dexhorthy closed this Mar 8, 2019
@dexhorthy
Copy link
Contributor

dexhorthy commented Mar 8, 2019

Wrong button whoops, didn't mean to close

@dexhorthy dexhorthy reopened this Mar 8, 2019
return {matched: false};
}

if (docCount === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this using a global variable?

@chase-replicated chase-replicated deleted the branch replicatedhq:main December 9, 2021 00:10
@chase-replicated chase-replicated changed the base branch from master to main December 9, 2021 02:35
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.

3 participants