Msp fix sourcelink git hash#433
Msp fix sourcelink git hash#433MaximilianSoerenPollak wants to merge 17 commits intoeclipse-score:mainfrom
Conversation
github-project-automation
bot
moved this from Draft
to Done
in INF - Infrastructure Community
License Check Results🚀 The license check job ran with the Bazel command: bazel run //src:license-checkStatus: Click to expand output |
github-project-automation
bot
moved this from Done
to In Progress
in INF - Infrastructure Community
|
The created documentation from the pull request is available at: docu-html |
| "//scripts_bazel:generate_sourcelinks", | ||
| "//src/extensions/score_source_code_linker", | ||
| ] + all_requirements, | ||
| pytest_config = "//:pyproject.toml", |
There was a problem hiding this comment.
Maybe also move this to different PR?
Had to enable pyrproject.toml as pytest.ini from score_pytest made it impossible to tests in ref-integration.
Though 'strictly' speaking has nothing to do with this PR
| # assert root.is_absolute(), f"Root path must be absolute. {root} is not" | ||
| #assert not file_path.is_absolute(), "File path must be relative to the root" | ||
| # assert file_path.is_relative_to(root), ( | ||
| # f"File path ({file_path}) must be relative to the root ({root})" | ||
| # ) |
There was a problem hiding this comment.
Need to rethink what asserts we can make here if any without blowing everything up
src/extensions/score_source_code_linker/generate_source_code_links_json.py
Outdated
Show resolved
Hide resolved
src/extensions/score_source_code_linker/generate_source_code_links_json.py
Outdated
Show resolved
Hide resolved
| test_module = ( | ||
| str(file).split("external/")[-1].split("/")[0].removesuffix("+") |
There was a problem hiding this comment.
Leave as is or split in 2 lines / commands for readability?
| assert root.is_absolute(), "Root path must be absolute" | ||
| assert not file_path.is_absolute(), "File path must be relative to the root" | ||
| # assert root.is_absolute(), f"Root path must be absolute. {root} is not" | ||
| #assert not file_path.is_absolute(), "File path must be relative to the root" |
There was a problem hiding this comment.
Commented code is bad. Commented asserts are even more weird to me.
There was a problem hiding this comment.
As said in my own comment.
These should either be commented in again or removed, that is correct.
I left them outcommented as it is still unclear which direction the PR goes. Once that is clear we can either remove or enable them again.
src/extensions/score_source_code_linker/generate_source_code_links_json.py
Outdated
Show resolved
Hide resolved
| # Module can be None if we are not in a combo build | ||
| if "external" in str(file): | ||
| test_module = ( | ||
| str(file).split("external/")[-1].split("/")[0].removesuffix("+") |
There was a problem hiding this comment.
This assumes Bazel internals. There should be a cleaner way which does not break if Bazel changes something there.
There was a problem hiding this comment.
I do very much agree. I hope there is a cleaner way to do it maybe via bazel actions or something unsure currently.
If there is this and may other things can be re-written cleaner and nicer in cooperation with the infra of bazel.
| # If external is in the filepath that gets parsed => | ||
| # file is in an external module => combo build | ||
| # e.g. .../external/score_docs_as_code+/src/helper_lib/__init__.py | ||
| if "external" in str(filepath): |
There was a problem hiding this comment.
What would happen if some module has an external folder in their repo?
I believe a more reliable check should be possible.
There was a problem hiding this comment.
This shouldn't be an issue as it splits on the left and maxsplit=1 therefore only the first external (which will always be the one before module name) will be removed, and if the module itself has some 'external' folder inside of it, that would be preserved.
But I will add a testcase to proof this.
| return f"{base_url}/blob/{current_hash}/{link.path}/{link.file}#L{link.line}" | ||
|
|
||
|
|
||
| def get_module_has_from_known_good_json(known_good_path: Path) -> dict[str, ModuleInfo]: |
There was a problem hiding this comment.
As we are not sure about the schema of known_good, we should have more checks and warnings/errors here to diagnose issues quickly (e.g. from some CI log).
There was a problem hiding this comment.
You mean the schema inside known good?
According to the ref-int guys the schema may change in some ways inside there, (more 2ndary keys or so) but the actual keys and structure should not change much.
Though if it does we will know.
Theoretically if we have a json schema that the 'known_good' needs to adhere to, to be parsed, would that make sense ?
And then test if the passed in known_good does adhere to that or error?
2 participants
Fixes source_code_linker to find and create correct links to source code & tests
📌 Description
Fixes source_code_linker to find and create correct links to source code & tests
🚨 Impact Analysis
✅ Checklist
I just wanted to ask for a architecture review of this @a-zw
Especially I'm a bit unhappy about how bazel is handeled and unsure if this is the correct way to approach this (with the hacky path appending / cutting).
But I could not think of another way except have two seperate solutions (for each build way).
Please let me know any and all feedback, nothing here is written in stone and I'm happy to change the implementation / architecture of it.