SCANPY-145 Dynamically infer the OS and Architecture properties#165
SCANPY-145 Dynamically infer the OS and Architecture properties#165guillaume-dequenne wants to merge 4 commits intomasterfrom
Conversation
guillaume-dequenne
force-pushed
the
SCANPY-145
branch
2 times, most recently
from
f2e583d to
a02a50d
Compare
guillaume-dequenne
force-pushed
the
SCANPY-145
branch
from
a02a50d to
644549f
Compare
guillaume-dequenne
changed the title
Seppli11
left a comment
Seppli11
left a comment
There was a problem hiding this comment.
The change itself looks good to me!
I've left two questions and put it back into "in progress"
| def mock_analysis_jres( | ||
| self, | ||
| body: Optional[list[dict]] = None, | ||
| os_matcher: Optional[str] = None, |
There was a problem hiding this comment.
I'm not sure I understand why this was removed...
There was a problem hiding this comment.
In absolute, it probably makes sense to keep it. However, doing so breaks the existing tests where no matcher is defined because we'll now always provide os and arch.
Since we already have quite a few tests for the configuration of the resolver, I felt it didn't really make sense to perform a complete refactoring of the tests and add more to have specific matchers for os/architectures, so I decided to drop it.
If you think we still need it, I can revisit and adapt the tests though.
There was a problem hiding this comment.
As discussed, I added the matchers back, with default values, as well as a small test using different OS/Arch combination to make sure that part works as well. Thanks for the feedback!
guillaume-dequenne
force-pushed
the
SCANPY-145
branch
from
cc247df to
3ef267e
Compare
guillaume-dequenne
force-pushed
the
SCANPY-145
branch
2 times, most recently
from
854a61d to
7a7b0f8
Compare
guillaume-dequenne
force-pushed
the
SCANPY-145
branch
from
7a7b0f8 to
dbb74bf
Compare
|
Seppli11
left a comment
Seppli11
left a comment
There was a problem hiding this comment.
LGTM!
Thanks for the quick changes. It's really nice that the SONAR_PROJECT_BASE_DIR path is explicit now.
|
Replaced by #173 |
2 participants
SCANPY-145
Due to the fact that our config is string based, I initially removed the
OsandArchenums. Seeing that this led to relying a bit too much on string literals, I added the second commit to keep these classes, simplifying tests in particular.It may be a bit overkill, so I left it as a separate commit in case you'd like to challenge the approach.