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

Add support for Go workspaces #999

Merged

Conversation

brunoapimentel
Copy link
Contributor

@brunoapimentel brunoapimentel commented May 10, 2024

This commit changes the resolve_gomod function so that it can handle a json stream coming from go list -m, which happens when workspaces are used in a Go project.

It also removes the previous restrictions on using workspaces.

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • New code has type annotations
  • [n/a] OpenAPI schema is updated (if applicable)
  • [n/a] DB schema change has corresponding DB migration (if applicable)
  • README updated (if worker configuration changed, or if applicable)
  • Draft release notes are updated before merging

@brunoapimentel brunoapimentel force-pushed the go-workspaces branch 2 times, most recently from 7ac5ce1 to 4fe7c95 Compare May 10, 2024 14:06
@brunoapimentel brunoapimentel force-pushed the go-workspaces branch 2 times, most recently from 3acda43 to 63d252b Compare May 21, 2024 03:34
@brunoapimentel brunoapimentel force-pushed the go-workspaces branch 2 times, most recently from e89852c to 391a749 Compare June 24, 2024 21:52
@brunoapimentel brunoapimentel marked this pull request as ready for review June 24, 2024 21:56
@brunoapimentel brunoapimentel requested review from chmeliik and removed request for taylormadore and ejegrova June 24, 2024 21:58
)

for module in workspace_modules:
module.version = main_module_version
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I could tell, Cachito is ignoring version tags applied to submodules. So we can just use the main module version for all workspaces, since we're pretty much keeping the current behavior.

We should probably fix this in a follow up PR, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it with:

{
  "repo": "https://github.com/cachito-testing/gomod-pandemonium",
  "ref": "0c6890c3280a00271891f4bd04705a56151428f0",
  "pkg_managers": ["gomod"]
}

And I got:

{
    "name": "github.com/cachito-testing/gomod-pandemonium/terminaltor",
    "purl": "pkg:golang/github.com%2Fcachito-testing%2Fgomod-pandemonium%[email protected]",
    "type": "library",
    "version": "v0.1.0"
}

Although the version for terminaltor should've been v1.0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(The SBOM also had an entry in which the version for terminaltor was ./terminaltor, so the behavior is a little odd)

Copy link
Contributor

@chmeliik chmeliik Jun 25, 2024

Choose a reason for hiding this comment

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

I think Cachito does support submodule versioning. But only if you tell it to process the submodule 🥲

i.e.

{
  "repo": "https://github.com/cachito-testing/gomod-pandemonium",
  "ref": "0c6890c3280a00271891f4bd04705a56151428f0",
  "pkg_managers": ["gomod"],
  "packages": {
    "gomod": [{"path": "."}, {"path": "terminaltor"}, {"path": "weird"}]
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I tried specifying all modules using the input you provided, and it still is correctly picking up for terminaltor:

  {
    "name": "github.com/cachito-testing/gomod-pandemonium/terminaltor",
    "purl": "pkg:golang/github.com%2Fcachito-testing%2Fgomod-pandemonium%[email protected]",
    "type": "library",
    "version": "v1.0.0"
  }

Which means we're still preserving the same behavior: submodule versions will only be picked up if the submodule is specified as a Cachito input package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see any duplication in the resulting SBOM that happens as a side effect of specifying all the submodules/workspaces present in the repo. The terminaltor module has a duplication regardless of what input packages you specify, though:

 {
    "name": "github.com/cachito-testing/gomod-pandemonium/terminaltor",
    "purl": "pkg:golang/github.com%2Fcachito-testing%2Fgomod-pandemonium%[email protected]",
    "type": "library",
    "version": "./terminaltor"
  },
  {
    "name": "github.com/cachito-testing/gomod-pandemonium/terminaltor",
    "purl": "pkg:golang/github.com%2Fcachito-testing%2Fgomod-pandemonium%[email protected]",
    "type": "library",
    "version": "v1.0.0"
  }

This does not happen with weird... I'm not sure what is causing it yet. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

On the one hand, with workspaces it doesn't make any sense to require that the user specify all submodules

On the other hand, ¯\_(ツ)_/¯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, so I found out that the previous solution was indeed creating duplicate results in the SBOM. I rewrote it so:

  • it picks the right version for each workspace
  • the user does not need to specify multiple input packages anymore

@brunoapimentel brunoapimentel force-pushed the go-workspaces branch 2 times, most recently from 0d6898d to 8274915 Compare July 3, 2024 22:14
@brunoapimentel
Copy link
Contributor Author

New push: rewrites the logic that parses the workspace data so that the versions applied to submodules are properly picked up.

"module_deps": main_module_deps,
"packages": main_packages,
}


def _set_local_modules_versions(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like the idea of updating the object attributes in place, but I also didn't want to do a deep dive into refactoring get_golang_version. This function takes a lot of attributes, and anything I wrote in the LocalModules class would bring all this crap together 😕

cachito/workers/pkg_managers/gomod.py Outdated Show resolved Hide resolved
cachito/workers/pkg_managers/gomod.py Show resolved Hide resolved
cachito/workers/pkg_managers/gomod.py Outdated Show resolved Hide resolved
@brunoapimentel brunoapimentel force-pushed the go-workspaces branch 2 times, most recently from a442a58 to a5c4200 Compare July 4, 2024 18:10
Go 1.18 introduced support for workspaces¹, which were until now
explicitly forbidden by Cachito when processing a request.

The main change this feature brings is the return value for the "go list
-m" command, which was used to retrieve the name of each of the modules
specified as input packages in a Cachito request.

This command now returns a list of all workspaces defined in the
"go.work" file, which we will use to properly list them as part of the
request's output.

¹ https://go.dev/ref/mod#workspaces

Signed-off-by: Bruno Pimentel <[email protected]>
Since the support for workspaces was not properly implemented, we had a
strict check on the existence of a "go.work" file in a repository, which
would then trigger a failure in a request.

Now that proper support is implemented, let's remove this check.

Signed-off-by: Bruno Pimentel <[email protected]>
Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

LGTM

Just to make sure I understand the limitations

  • all the workspaces will be reported with correct versions, doesn't matter if the user tells cachito to process just one workspace or all of them
  • packages from a workspace will only be reported if the user tells cachito to process that workspace

More or less?

@brunoapimentel
Copy link
Contributor Author

* all the workspaces will be reported with correct versions, doesn't matter if the user tells cachito to process just one workspace or all of them

Yes, because go list -m will always list all workspaces, and we're running _get_golang_version individually for each one of them.

  • packages from a workspace will only be reported if the user tells cachito to process that workspace

Yes, because of the limitation of go list ./....

@brunoapimentel brunoapimentel added this pull request to the merge queue Jul 8, 2024
Merged via the queue into containerbuildsystem:master with commit c36a7da Jul 8, 2024
14 checks passed
@brunoapimentel brunoapimentel deleted the go-workspaces branch July 8, 2024 17:34
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.

4 participants