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

Use Ubuntu 22.04 (Jammy) for Docker image #977

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

rhuitl
Copy link
Contributor

@rhuitl rhuitl commented May 1, 2023

@xtreme-shane-lattanzio FYI, not sure if all the tests will pass.

Update:

  • Python2 is no longer available. Do we need it? I changed the default in the code to use "3" and that seems to make the tests pass.
  • The Go tool dep is deprecated since 2020 and not available on Jammy. See also https://github.com/golang/dep#dep. Do we need it?

Somewhat related:
In general, it might be worth considering not to build one gigantic Docker image that has all possible package managers and supporting tools and frameworks in it, but rather make LicenseFinder itself use smaller Docker images to perform the scans.
This way, it could use the most up-to-date, unmodified upstream images (e.g. https://hub.docker.com/_/maven, https://hub.docker.com/_/python, etc.) or even ancient ones with legacy software. Further, LicenseFinder users would only have to pull (or wait for LF to pull) the Docker images they actually need.

@cf-gitbot
Copy link
Collaborator

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

@rhuitl rhuitl force-pushed the jammy branch 2 times, most recently from 2bff546 to 3458bb0 Compare May 2, 2023 17:00
* Use Python3 by default, Python2 is no longer available
* Ignore "dep" tests for Go for now as "dep" is no longer available
@xtreme-shane-lattanzio
Copy link
Contributor

Hey @rhuitl ! I am looking into some things internally to ensure that we can drop support for dep and python2. We build other images off of the LF image so we will need to bump them as well and then change tests for those apps to also not use these old tools. I think it should be okay but need to double check. This is a pretty big change but it does make sense to do and then release an 8.0.

In the mean time, there are some tests failing for the default python and pip versions. It is also not a big deal, but I'm not sure if there was a functional reason why you moved the flutter stuff?

As for breaking down the image, that would require a lot of changes for us so I would still want a single big one but I am not against adding more dockerfiles in addition to satisfy what you are talking about.

Thanks again for adding this!

@rhuitl
Copy link
Contributor Author

rhuitl commented May 3, 2023

I moved the Flutter stuff down because I think it was accidentally put in between a bigger block for Swift. If you look closely, it installs dependencies for Swift, then came Flutter, then the second part of the Swift section.

For the tests I'm not sure when I'll get to look at them. It's a bit painful to run these on my local machine, kind of slow and the commands to run the tests rarely just work. Sometimes, I edit the spec files to not glob *spec but only the ones I want... and not being good in Ruby doesn't make this task easier. If somebody can lend a hand and fix or remove the tests, I'd be very happy :-)

@xtreme-shane-lattanzio
Copy link
Contributor

The tests that are failing are pretty trivial to fix. It is just because pip and python are using 3 instead of 2 so now the expect commands are saying for example:

expected: ("python /tmp/build/aa1bbb9d/LicenseFinder/bin/license_finder_pip.py some-file.txt")
got: ("python3 /tmp/build/aa1bbb9d/LicenseFinder/bin/license_finder_pip.py some-file.txt")

This is in a few places but you can just make the change and push and see the pipeline go green. The integration tests weren't run because the unit tests were red but we can tackle that next.

The only other failure is it { expect(Scanner::PACKAGE_MANAGERS).to include described_class } because you removed Dep from the list so that one should be an easy fix as well.

Again, thanks for your work on this!

@rhuitl
Copy link
Contributor Author

rhuitl commented May 4, 2023

Ah thanks, and about the "Dep", where does it get the described_class from? Do I have to remove all files related to "Dep"?

@xtreme-shane-lattanzio
Copy link
Contributor

Unfortunately if I remember correctly, the described class in this case is anything that extends the PackageManager class. So you would either need to remove the extend, comment out the class, or remove it. That would definitely case some integration tests to fail which would be in the testing_dsl.rb. There are some Dep test projects in there.

I forget if there is more than that that will cause trouble. I know there are dep fixtures but if you remove the link to package managers, I imagine they will never be looked at.

@xtreme-shane-lattanzio
Copy link
Contributor

Hey @rhuitl ! The bionic deprecation seems to be causing new issues. I am wondering if you are able to do the dep removal? Let me know! I am working internally here to see if anything else needs to be done for our tools before we merge and release this. Thanks!!

@xtreme-shane-lattanzio
Copy link
Contributor

@rhuitl FYI I opened up #992 so that I can tinker around with this as well and hopefully get the upgrade through!

@rhuitl
Copy link
Contributor Author

rhuitl commented Jul 17, 2023

@rhuitl FYI I opened up #992 so that I can tinker around with this as well and hopefully get the upgrade through!

Hi Shane, that's great! I hope it'll be easy, but as you said, it's probably mostly about deciding whether we can remove some older package managers that are no longer available and then remove all related code to make the tests happy.

In the past weeks, I've been busy with non-tech stuff and didn't have time to look into this further.

@xtreme-shane-lattanzio xtreme-shane-lattanzio merged commit da637b8 into pivotal:master Jul 17, 2023
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