Switch to packs#10
Conversation
alexevanczuk
left a comment
alexevanczuk
left a comment
There was a problem hiding this comment.
Thanks for this awesome contribution! Everything looks great – I'm just not sure about no longer checking in Gemfile.lock. Had some questions about that. If you wanted to continue to chat through it, I'd recommend we pull that change into a separate PR – that way this could be unblocked and we can go ahead and merge it!
Lastly, want to bump the minor version in the gemspec? That way CD will automatically publish a new release once this merges on main and CI passes.
| /pkg/ | ||
| /spec/reports/ | ||
| /tmp/ | ||
| Gemfile.lock |
There was a problem hiding this comment.
Chore: Remove Gemfile.lock from VCS. Although the Bundler team now recommends on having it, that only makes sense with additional tooling in place (e.g. Dependabot). Without them, this is only making it harder to test against your dependencies and validate your gemspec properly.
Could you explain more context here? I think this is still useful to have in VCS to ensure all contributors are using the same set of dependencies, especially as it's recommended by bundler. In what ways are you finding it's harder to test against your dependencies and validate your gem spec?
There was a problem hiding this comment.
Following your suggestion, I'll push the Gemfile.lock back into VCS and open a separate issue about this 👍
|
|
||
| # Building a team graph for specific teams | ||
| ``` | ||
| ```ruby |
| spec.add_dependency 'rake' | ||
| spec.add_dependency 'ruby-graphviz' | ||
|
|
||
| spec.add_development_dependency 'bundler', '~> 2.2.16' |
There was a problem hiding this comment.
I think we'd still want this if we choose to continue to commit the Gemfile.lock.
|
Also looks like type check is failing! Let me know if I can help with that 😄 |
|
Of course, I forgot to re-run |
ParsePackwerk is still needed to parse packages dependencies and violations. However, CodeOwnership will now expect us to pass it Packs::Pack objects. A direct effect of this transition is that we now need to remove the root package from processing, since Packs does not consider that.
iMacTia
force-pushed
the
mg/switch-to-packs
branch
from
c1bd3d8 to
ea2d4d8
Compare
|
@alexevanczuk |
alexevanczuk
left a comment
alexevanczuk
left a comment
There was a problem hiding this comment.
Looks great, thank you!
2 participants
WHY
We recently updated
parse_packwerk,code_ownershipand other gems in therubyatscaleecosystem.They all moved to make
packsa more central part of it, but this gem has not been updated yet, which causes issues like the following:WHAT
code_ownership.packs, we now need to ignore the root package, but that was already in the plans based on comments I found in the code.Chore: RemoveUpdateGemfile.lockfrom VCS.code_ownershipversion inGemfile.lockTesting
Since this repo has no tests, I manually tested the following steps:
VisualizePackwerk.package_graph!as demonstrated in the READMEVisualizePackwerk.team_graph!as demonstrated in the READMEmodularity-graphas explained in thed3_graph_generator/README.mdNOTE:
They were all successfully working against our codebase.
However, it's worth noting that we use an older version of
parse_packwerk(0.14.0) since we're still using Packwerk 2.It would be great to have someone else testing these changes against a Packwerk 3 codebase using the latest
parse_packwerk.