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

Do not error on rocky linux referenced deps #1473

Merged
merged 3 commits into from
Sep 20, 2024
Merged

Do not error on rocky linux referenced deps #1473

merged 3 commits into from
Sep 20, 2024

Conversation

gilfaizon
Copy link
Contributor

@gilfaizon gilfaizon commented Sep 20, 2024

Overview

If using rocky as an os in the fossa-deps.yml, fossa will fail to read the fossa-deps file with the following error:

[ERROR] An issue occurred

  *** Relevant Errors ***

      Error: parsing file: path/to/fossa-deps.yml
        Aeson exception:
        Error in $['referenced-dependencies'][1]: Provided os: rocky is not supported! Please provide oneOf: alpine, centos, debian, redhat, ubuntu, oraclelinux, busybox, sles, fedora
      Support: If you believe this to be a defect, please report a bug to FOSSA support at https://support.fossa.com, with a copy of: /path/to/fossa-deps.yml

This aims to fix that, and allow the use of rocky for rpm-generic deps. It also updates the fossa-deps schema.

Acceptance criteria

FOSSA CLI analysis does not fail when using rocky as an os value.

Testing plan

I created a fossa-deps.yml with the follwing:

referenced-dependencies:
- name: binutils
  type: rpm-generic
  arch: x86_64
  os: rocky
  osVersion: '8.6'
  version: 0:2.30-113.el8

This will fail on fossa-cli versions outside of this branch.

I tested by downloading the built MacOS arm build from this run:

My scan was successful, and the dependency was included in my FOSSA project.

Risks

I looked at this very quickly; I may have missed a spot where I need to update the OS, or provide additional info.

References

N/A

Checklist

  • I added tests for this PR's change (or explained in the PR description why tests don't make sense).
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • If this PR added docs, I added links as appropriate to the user manual's ToC in docs/README.ms and gave consideration to how discoverable or not my documentation is.
  • If this change is externally visible, I updated Changelog.md. If this PR did not mark a release, I added my changes into an # Unreleased section at the top.
  • If I made changes to .fossa.yml or fossa-deps.{json.yml}, I updated docs/references/files/*.schema.json AND I have updated example files used by fossa init command. You may also need to update these if you have added/removed new dependency type (e.g. pip) or analysis target type (e.g. poetry).
  • If I made changes to a subcommand's options, I updated docs/references/subcommands/<subcommand>.md.

@gilfaizon gilfaizon marked this pull request as ready for review September 20, 2024 17:21
@gilfaizon gilfaizon requested a review from a team as a code owner September 20, 2024 17:21
@spatten
Copy link
Contributor

spatten commented Sep 20, 2024

@csasarak does your work on Core affect this ticket at all?

https://github.com/fossas/FOSSA/pull/13898

@csasarak
Copy link
Contributor

csasarak commented Sep 20, 2024

I don't think it does directly because in this case we're validating the OS against a list internal to the CLI. We actually do support Rocky Linux in our backend so I think it's OK to merge this PR.

However, the future "correct" solution I think is to either not validate or fetch a list of valid distributions at some point during analysis and validate against that. In the future if we're able to more easily add support for other Linux distributions we don't want to be forced to coordinate new CLI releases with the addition of that support. So I think I'd also be good with a version of this that either warns or doesn't do the check at all.

@spatten If you want to we can open up a discussion internally but I think the above is what we ought to do.

@csasarak
Copy link
Contributor

I guess one possible reason to do the validation here is to potentially lessen the load on Core. I think if Core can't handle that we should figure out a way to fix it rather than maintaining this logic IMO.

@spatten
Copy link
Contributor

spatten commented Sep 20, 2024

I guess one possible reason to do the validation here is to potentially lessen the load on Core. I think if Core can't handle that we should figure out a way to fix it rather than maintaining this logic IMO.

However, the future "correct" solution I think is to either not validate or fetch a list of valid distributions at some point during analysis and validate against that. In the future if we're able to more easily add support for other Linux distributions we don't want to be forced to coordinate new CLI releases with the addition of that support. So I think I'd also be good with a version of this that either warns or doesn't do the check at all.

I agree with that. This PR is still worth merging, but in the future we should skip validation or move the validation to Core

Copy link
Contributor

@spatten spatten left a comment

Choose a reason for hiding this comment

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

This looks good to me! I left a comment about formatting of the Changelog, but nothing blocking at all

Changelog.md Outdated
@@ -1,3 +1,7 @@
# Unreleased

- fossa-deps: Fixed an issue where Rocky linux deps could skip the fossa-deps file during analysis
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a link to the PR? Also, is that an accurate description of what we're fixing. Based on my reading, it's something like "Fixed an issue where Rockly Linux deps were not supported in the fossa-deps file"

Suggested change
- fossa-deps: Fixed an issue where Rocky linux deps could skip the fossa-deps file during analysis
- fossa-deps: Fixed an issue where Rocky Linux deps were not supported in the fossa-deps file ([#1473](https://github.com/fossas/fossa-cli/pull/1473))

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, # Unreleased should go below # FOSSA CLI Changelog and be a level 2 header: ## Unreleased

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! That's a better description!

I also updated the PR template to make this more intuitive. It originally said "# Unreleased section at the top," but my markdown senses were tingling.

@gilfaizon gilfaizon enabled auto-merge (squash) September 20, 2024 21:25
@gilfaizon gilfaizon merged commit ea1991c into master Sep 20, 2024
19 checks passed
@gilfaizon gilfaizon deleted the david-rocky branch September 20, 2024 21:26
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