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

Move tools an acceptance to repository root #327

Merged
merged 2 commits into from
Jan 10, 2023

Conversation

zregvart
Copy link
Member

@zregvart zregvart commented Jan 2, 2023

This sets up the acceptance tests as a separate go module and moves acceptance and tools from internal to repository root to better reflect them as modules.

@codecov
Copy link

codecov bot commented Jan 2, 2023

Codecov Report

Merging #327 (c5b3bc2) into main (527202f) will decrease coverage by 0.77%.
The diff coverage is n/a.

❗ Current head c5b3bc2 differs from pull request most recent head c7b267d. Consider uploading reports for the commit c7b267d to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #327      +/-   ##
==========================================
- Coverage   77.33%   76.56%   -0.78%     
==========================================
  Files          45       45              
  Lines        3433     3358      -75     
==========================================
- Hits         2655     2571      -84     
- Misses        778      787       +9     
Flag Coverage Δ
acceptance 52.79% <0.00%> (+0.08%) ⬆️
generative 4.71% <0.00%> (ø)
integration 90.48% <0.00%> (ø)
unit 60.66% <0.00%> (-1.21%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
internal/evaluator/conftest_evaluator.go 72.04% <0.00%> (-12.70%) ⬇️

@zregvart zregvart force-pushed the pr/go-workspaces branch 2 times, most recently from d7b8eac to 82c2aa3 Compare January 3, 2023 09:09
@lcarva
Copy link
Member

lcarva commented Jan 3, 2023

This seems pretty straight forward. If using go 1.18 or newer, it doesn't seem to require any special usage. It just works.

Are there any downsides to this approach? I wonder if there will be any issues with dependabot.

@zregvart
Copy link
Member Author

zregvart commented Jan 4, 2023

Are there any downsides to this approach? I wonder if there will be any issues with dependabot.

Seems that dependabot doesn't support workspaces right now, so that's a downside -- I've seen go mod tidy in a module update the go.work.sum file.

Other than that, I was hoping that we could work with dependencies a bit more independent, but that is not the case, so having modules without workspace could be a bit more helpful at the moment. For example, the acceptance tests could run with a different version of docker go API than the CLI, which would help with Testcontainers update (#325). Downside of that, is that I don't think it's good for the projects to drift in versions, an example of that would be tekton/cli vs tekton/pipeline version issue that occurred in #324.

I haven't seen anything that would impede the day to day workflow. We don't really need workspaces, the modules don't depend on each other. An interesting development could be if we were to merge the https://github.com/hacbs-contract/enterprise-contract-controller and https://github.com/hacbs-contract/ec-cli into a single git repository, here workspace would be very beneficial, so if we would like to consider that, this would be a good first step.

@simonbaird
Copy link
Member

I can see the "move acceptance tests to their own directory with their own go.mod file" change, but what part of this change is the "create go workspace"?

@zregvart
Copy link
Member Author

zregvart commented Jan 4, 2023

but what part of this change is the "create go workspace"?

In essence the go.work and go.work.sum files (https://github.com/hacbs-contract/ec-cli/pull/327/files#diff-70f7e3c361ae136c655130ab253927b5f6e5c987797a095db1bbb1e790930484). The repercussions are in the go.mod files. With go work sync the dependencies get hoisted (or that's my understanding) from modules and then the (newest?) version gets synced down to other modules -- resulting in some version changes that I did not expect when starting on this.

We don't inter-depend between modules so no other changes are visible. If we were, the replace in go.mod that would point to a local directory would be removed.

@simonbaird
Copy link
Member

Aha, thanks.

Copy link
Member

@simonbaird simonbaird left a comment

Choose a reason for hiding this comment

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

Approve FWIW, but I don't have much insight into the pros and cons.

@lcarva
Copy link
Member

lcarva commented Jan 10, 2023

Have we decided to drop this, at least for now?

@zregvart
Copy link
Member Author

Have we decided to drop this, at least for now?

I'll remove the go.work and go.work.sum files & update the title/commit message.

@zregvart zregvart changed the title Create go workspace with cli, acceptance and tools Move tools an acceptance to repository root Jan 10, 2023
.github/workflows/checks.yaml Show resolved Hide resolved
This sets up the acceptance tests as a separate go module and moves
acceptance and tools from `internal` to repository root to better
reflect them as modules.
@zregvart zregvart merged commit dfb8de1 into enterprise-contract:main Jan 10, 2023
@zregvart zregvart deleted the pr/go-workspaces branch January 10, 2023 21:53
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