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

Improve findsources functionality #51

Merged
merged 3 commits into from
Jan 28, 2024
Merged

Improve findsources functionality #51

merged 3 commits into from
Jan 28, 2024

Conversation

sachinshaji
Copy link

Conduct source URL discovery using sw360 lookup, perform extensive GitLab deep search, and adapt search strategy based on diverse programming languages.

@gernot-h
Copy link
Collaborator

@sachinshaji, can you please explain the current state of this PR? I see you marked it as draft, does it make sense already to review your changes?

pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@sachinshaji sachinshaji marked this pull request as ready for review December 19, 2023 11:58
…ive GitLab deep search, and adapt search strategy based on different programming language
Comment on lines 49 to 51
if exit_no_login:
print_red(" No SW360 server URL specified!")
sys.exit(ResultCode.RESULT_ERROR_ACCESSING_SW360)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that you could also catch SystemExit in the caller of login(). Not sure if we want an extra parameter for this. What do you think, @tngraf?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see no reason for an extra parameter.
@sachinshaji Please explain why this should be needed.

Copy link
Author

Choose a reason for hiding this comment

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

As we have added the functionality to lookup the SW360 server(to find the already existing source code) for the capycli bom findsources command we need SW360 URL and TOKEN. If we don't add the exit_no_login paramerter to login the capycli bom findosurces will always search for SW360 URL and TOKEN and if we don't give, the command will fail/error out.

We have added the exit_no_login so that even the users doesn't give SW360 creds the capycli bom findsources will run successfully trying to find the source code from github(only skip the SW360 lookup).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I stll do not see the need:

  • Case 1: a token and a SW360 url have been given on the command line.
    You can check that they are not emptry and then call login() => no exit_no_loginneeded.
  • Case 2: neither token nor SW360 url have been given on the command line.
    a) The user decided not to provide this information => user does not want to use SW360 information
    => do not call login => no exit_no_loginneeded.
    b) You can mimik the behavior of login() and check if the information is available in the environment.
    If is exists, pass it as parameter. If it does not exist you cannot login.
    In either way no exit_no_loginneeded.

Copy link
Author

Choose a reason for hiding this comment

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

Ok I will check this. Thanks for the detailed explanation.

@gernot-h
Copy link
Collaborator

gernot-h commented Dec 20, 2023

@sachinshaji, can you please check the results of the unit tests here? It seems there are some errors and a warning that semver.compare is deprecated and shouldn't be used any more.

@sachinshaji
Copy link
Author

@gernot-h I will check the warning message we have. 'removesuffix' attribute is not present under string class in python 3.8. Is it fine to use that?

@gernot-h
Copy link
Collaborator

gernot-h commented Dec 20, 2023

@gernot-h I will check the warning message we have. 'removesuffix' attribute is not present under string class in python 3.8. Is it fine to use that?

As you only use that once in your code, I guess it's not justifying dropping Python 3.8 support for that. I think it should be easily replaceable by something like:

if your_string.endsWith(suffix):
    your_string = your_string[:-len(suffix)]

@sachinshaji
Copy link
Author

@gernot-h The warning messages and errors are resolved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Things to improve:

  • Provide help text for the arguments -t, -url
  • Improve text coverage
  • Add type information (... mypy will come soon...)

Copy link
Author

Choose a reason for hiding this comment

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

New commit in PR has been initiated with these comments addressed

@tngraf tngraf merged commit 6ba1391 into sw360:main Jan 28, 2024
1 of 5 checks passed
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