-
Notifications
You must be signed in to change notification settings - Fork 15
Merge individual modules to a single score_tooling module #61
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR consolidates multiple individual Bazel modules into a single score_tooling module, simplifying dependency management and module structure. The changes update module references, file paths, and dependencies to reflect the new unified module architecture.
Key changes include:
- Removal of individual MODULE.bazel files from separate tool directories
- Creation of a unified MODULE.bazel at the root level defining the
score_toolingmodule - Updates to all references from individual modules (e.g.,
@score_python_basics,@score_linter) to the unified@score_toolingmodule
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| MODULE.bazel | Creates the unified score_tooling module with consolidated dependencies |
| tools/sample.sh | Updates tool references to use the unified module structure |
| tools/README.md | Updates documentation to reflect the new module structure |
| tools/MODULE.bazel | Removes individual module definition (now consolidated) |
| starpls/integration_tests/starpls_test.py | Updates binary path reference for the new module structure |
| starpls/integration_tests/BUILD | Updates load statements and visibility for unified module |
| starpls/MODULE.bazel | Removes individual module definition |
| python_basics/score_pytest/py_pytest.bzl | Updates internal references to use unified module paths |
| python_basics/defs.bzl | Updates load statements and data references |
| dash/tool/formatters/dash_format_converter.bzl | Updates tool label reference |
| cr_checker/cr_checker.bzl | Updates source reference to unified module |
| BUILD | Removes individual module definition, adds package visibility |
AlexanderLanin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid we need to move all bzl (and potentially BUILD) files as well. So the users do not need to use paths which are internal to tooling repository when they use one of our bzl files.
However we could also do that as a 2nd PR, just to keep them manageable??
MODULE.bazel
Outdated
|
|
||
| module( | ||
| name = "score_tooling", | ||
| version = "1.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this has some likelihood of not working on the first attempt I'd rather make it a pre-release.
| version = "1.0.0", | |
| version = "1.0.0-RC1", |
MODULE.bazel
Outdated
| module( | ||
| name = "score_tooling", | ||
| version = "1.0.0", | ||
| compatibility_level = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must match major version.
| compatibility_level = 0, | |
| compatibility_level = 1, |
I think that sounds reasonable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you should delete this?
This is needed for the integration tests and should be treated as a seperate module. Unsure if this will allow the tests to run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, we can look at the (few) integration tests
load("@score_tooling//python_basics:defs.bzl", "score_virtualenv")
I'd rather users have:
load("@score_tooling//python.bzl", "score_virtualenv")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed @AlexanderLanin and @MaximilianSoerenPollak
Let's do it in a separate follow up PR.
tools/README.md
Outdated
| local_path_override( | ||
| module_name = "score_tooling", | ||
| path = "../tooling", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not have a local path override, or am I seeing it wrong?
MODULE.bazel
Outdated
| bazel_dep(name = "rules_python", version = "1.4.1") | ||
| bazel_dep(name = "aspect_rules_py", version = "1.4.0") | ||
| bazel_dep(name = "aspect_rules_lint", version = "1.4.2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at docs-as-code main branch to see what the right version numbers are.
As we are not using latest, but I think some slightly newer ones
Merge individual modules to a single score_tooling module Change-Id: I73d1ea073aa166403195a3cc47fe28f98e004513
AlexanderLanin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's start moving in this direction
originates from CR #134160