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

Support mixing remote and local configs in EXTENDS #2533

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Kurt-von-Laven
Copy link
Collaborator

Fixes #2371.

EXTENDS accepts both absolute URLs and relative file paths to config files to inherit from. Stop assuming that all relative file paths are relative to the current workspace. This assumption does not hold when parsing a config inherited from a different repository. This situation arises most simply when A inherits from B via an absolute URL, and B inherits from C via a relative file path. Both inherited config files, B and C, are in a different repository than A.

Proposed Changes

  1. Make a best effort to infer the URL of the repository root, and use that to correctly resolve relative file paths within that repository recursively.
  2. Support 16 of the most popular Git hosting services from all over the world initially, and raise an error for unsupported cases.
  3. Add mypy types to combine_config.
  4. Decompose retrieving a config from an absolute URL into a new helper function named download_config, and reuse it.
  5. Prefer the high-level, object-oriented pathlib.Path to the low-level, functional os.path.

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@nvuillam
Copy link
Member

nvuillam commented Apr 6, 2023

🦙 MegaLinter status: ⚠️ WARNING

Descriptor Linter Files Fixed Errors Elapsed time
✅ BASH bash-exec 6 0 0.01s
✅ BASH shellcheck 6 0 0.19s
✅ BASH shfmt 6 0 0 0.43s
✅ COPYPASTE jscpd yes no 3.24s
✅ DOCKERFILE hadolint 116 0 19.29s
✅ JSON eslint-plugin-jsonc 21 0 0 2.42s
✅ JSON jsonlint 19 0 0.28s
✅ JSON v8r 21 0 14.87s
⚠️ MARKDOWN markdownlint 312 0 230 7.71s
✅ MARKDOWN markdown-link-check 312 0 5.57s
✅ MARKDOWN markdown-table-formatter 312 0 0 20.32s
✅ OPENAPI spectral 1 0 1.7s
⚠️ PYTHON bandit 185 54 2.99s
✅ PYTHON black 185 0 0 5.32s
✅ PYTHON flake8 185 0 4.51s
✅ PYTHON isort 185 0 0 0.87s
✅ PYTHON mypy 185 0 8.48s
✅ PYTHON pylint 185 0 13.41s
⚠️ PYTHON pyright 185 252 20.05s
✅ PYTHON ruff 185 0 0 0.45s
✅ REPOSITORY checkov yes no 38.61s
✅ REPOSITORY git_diff yes no 0.39s
✅ REPOSITORY secretlint yes no 15.17s
✅ REPOSITORY trivy yes no 35.84s
✅ SPELL cspell 753 0 27.78s
✅ SPELL misspell 572 0 0 0.95s
✅ XML xmllint 3 0 0 0.42s
✅ YAML prettier 81 0 0 3.18s
✅ YAML v8r 23 0 69.77s
✅ YAML yamllint 82 0 1.27s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security

@bdovaz
Copy link
Collaborator

bdovaz commented Apr 9, 2023

I haven't looked at the code much because I'm on vacation and from my cell phone but:

  • Azure DevOps is missing in the urls to parse.
  • Clearly test is needed because it is somewhat complex to verify and easy to create regressions.

@Kurt-von-Laven
Copy link
Collaborator Author

Kurt-von-Laven commented Apr 9, 2023

Definitely agree on both points. When you return from vacation, can you please either let me know what repo_root_index to use for Azure DevOps URL, or give me an example URL to the raw contents of a file in a repository hosted in Azure DevOps? I just need the structure of the URL, so any identifying information can be changed.

As an example, the repo_root_index of https://raw.githubusercontent.com/ScribeMD/.github/0.9.10/.github/base.mega-linter.yaml is 4. The path portion of the URL is /ScribeMD/.github/0.9.10/.github/base.mega-linter.yaml, which gets split into parts as ("/", "ScribeMD", ".github", "0.9.10", ".github", "base.mega-linter.yaml"). We want to slice off .github/base.mega-linter.yaml, which is the path within the repository to the MegaLinter config, so we want to index the parts with [:4].

EXTENDS accepts both absolute URLs and relative file paths to config
files to inherit from. Stop assuming that all relative file paths are
relative to the current workspace. This assumption does not hold when
parsing a config inherited from a different repository. This situation
arises most simply when A inherits from B via an absolute URL, and B
inherits from C via a relative file path. Both inherited config files,
B and C, are in a different repository than A. Make a best effort to
infer the URL of the repository root, and use that to correctly resolve
relative file paths within that repository recursively.
@bdovaz
Copy link
Collaborator

bdovaz commented Apr 12, 2023

@Kurt-von-Laven I've been doing some research and it turns out that in Azure DevOps there is no "easy" way to get a raw url to a repository file. That is, there is no "raw" button like on GitHub.

So there are 2 alternatives:

  1. put the url to the file through the Azure DevOps web interface. Example:

https://dev.azure.com/MY_ORGANIZATION/MY_PROJECT/_git/MY_REPOSITORY?path=/test/path/file.yml

  1. Put the url in "raw" format which is to put the URL to the a specific Azure DevOps API endpoint.

The following link discusses the 2 cases: https://stackoverflow.com/a/59547731

I imagine that the first case is the one to be addressed.

@bdovaz
Copy link
Collaborator

bdovaz commented Apr 30, 2023

@Kurt-von-Laven did you read my message?

@Kurt-von-Laven
Copy link
Collaborator Author

Kurt-von-Laven commented May 17, 2023

Sorry for the slow reply, @bdovaz.

I am tempted, since we are doing a major release, to switch the format of EXTENDS from a string to { base: string, relative_path: string } (or { repo: string, relative_path: string }).

How do folks feel about this (cc: @nvuillam, @echoix)?

It would allow easy support for every file hosting platform I am familiar with (including Azure DevOps based on @bdovaz's helpful research), because we would always be able to determine the full URL to the MegaLinter file if it was not in the same repository.
It also makes more conceptual sense to me from a design perspective since a single string fundamentally doesn't convey enough information to properly implement extending a remote config that extends a local config (c.f., #2371).

A config file in the same repo could be extended with { relative_path: string } (or even simply as a string).

The main alternatives I see if we prefer backwards compatibility are (a) the idea in the PR where we guess base and relative_path for an ever-expanding number of recognized file/repository-hosting platforms; (b) support either string or { base: string, relative_path: string }, which results in more complexity and guarantees that string continues to be handled improperly when mixing absolute and relative paths in an inheritance hierarchy; or (c) both (a) and (b), which would work great but also be the most complex to implement and maintain.

@echoix
Copy link
Collaborator

echoix commented May 17, 2023

I agree with you that guessing is a recipe to at least understand it wrong once. As an advanced feature (less users) + a major version change, it's reasonable to try to do the right thing.

Label as breaking, and we make sure to have it ready on time?

@nvuillam
Copy link
Member

@Kurt-von-Laven I'd be ok with such evolution, but I think it's better if we also keep ascending compatibility about configuration

In the jsonschema you can define a property to be a simple string, or an object :)

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this pull request should stay open, please remove the O: stale 🤖 label or comment on the pull request.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jun 17, 2023
@github-actions github-actions bot closed this Jul 1, 2023
@bdovaz bdovaz reopened this Jul 1, 2023
@bdovaz
Copy link
Collaborator

bdovaz commented Jul 1, 2023

@Kurt-von-Laven are you still working on this?

@github-actions github-actions bot removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jul 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this pull request should stay open, please remove the O: stale 🤖 label or comment on the pull request.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Aug 2, 2023
@bdovaz bdovaz added nostale This issue or pull request is not stale, keep it open and removed O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity labels Aug 13, 2023
@bheemvennapureddy
Copy link
Contributor

Is some one working on this @nvuillam - Can we use extends now with the current version ?

@nvuillam
Copy link
Member

@bheemvennapureddy extends works today with remote configs :)

https://megalinter.io/latest/config-variables/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nostale This issue or pull request is not stale, keep it open
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't Extend A Remote Config That Extends A Local Config
5 participants