-
-
Notifications
You must be signed in to change notification settings - Fork 17.6k
structurizr-cli: init at 2025.05.28 #417630
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
Conversation
5acce2e to
cf42ee0
Compare
RossSmyth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far. I would recommend just running nix fmt pkgs/by-name/st/structurizr/package.nix to get formatting.
Here's some recommendations.
|
Also |
drupol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
Thanks for the PR, I made a few comments, let me know if this is not clear.
|
Update: I did have a look at some of the remarks, not everything yet. Thanks for all the feedback, I was first doubting to push a PR, certainly since I really don't feel that knowledgeable about nix / functional programming, but I feel I'm already learning a lot in the process! |
Welcome! If you want any real-time help, you can reach out to me on Matrix and I'd be happy to help out. Or just ask in the Matrix channels for general help, people are usually friendly. |
|
I submitted a PR to upstream so that |
|
Oh nice. If it the patch can, you could apply that PR as a patch until a release is made with it merged with patches = [
(fetchPatch2 {
url = "https://patch-diff.githubusercontent.com/raw/structurizr/cli/pull/175.patch";
hash = "";
})
]; |
|
What do you mean ? |
Ah, you were already working on this? I was at some point also thinking of seeing how to build it from source, such I would have a nix flake as part of the structurizr-cli repo -- but I didn't think I'd know enough about java nor nix, never mind the combination of these two. Would you rather wait for your PR to be merged in and then be able to build from source instead? |
|
OK, I think at this point I did address all of the issues and the overall result definitely looks better. Should I still squash everything myself or can that be done on merge? |
The package should be structurizr-cli. Please update the PR title (please read the guidelines on how to properly format the commits and the PR title), the package directory and squash the commits into one single commit. At the end there should only be 2 commits in the PR. |
a3bb34c to
ed39be2
Compare
ed39be2 to
05a4257
Compare
Thanks for the effort! I'm a bit mixed about it though, since it depends on all these workarounds just be able to build from source. On the other hand, that's a bit the magic that I think however though it would be better if these sort of fixes are handled at the original source repository. Would you mind if we;d go ahead with the MR, built from the release instead and only reconsider this later on again? |
As far as I'm concerned, it's OK like this, with the caveat of my reply in #417630 (comment) i.e. it looks like it's possible to also build from source with some patches, but I don't know if that's really required / desired. |
|
Building from source is always preferred indeed. |
Which is why I proposed a PR to upstream to adopt better gradle practices. Adding the patch temporarily isn't that much of a workaround IMO. Except for that I don't really see a lot of hacks. Maybe |
|
Since consensus seemed to be to build from source with the patch (and since @dtomvan pretty much did all the work), I did switch to that version. I did have to do some further tweaks, like generate the In that sense, I'm not completely sure what I did -- but I tend to have this feeling for anything nix-related at the moment still 😅 If this is OK, I will squash / recommit and it's ready for final review (?) |
drupol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're almost there, make sure this PR only contains 2 commits.
- 1 commit for adding you in the maintainers list
- 1 commit for adding
structurizr-cli
6639686 to
59ad694
Compare
59ad694 to
a62dc97
Compare
|
To merge; should I call the merge bot? @NixOS/nixpkgs-merge-bot merge |
|
@mhemeryck merge not permitted (#305350): |
|
Looks like it shouldn't be merged, it's failing on the CI. |
c7d0cfd to
ad2a096
Compare
|
Looks like the hash of the patches are wrong: |
I did try to get the hashes through But this doesn't look OK; is there another way to do it? |
|
I think you first need to download the file, then use |
drupol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please squash the commits ?
Adds [`structurizr-cli`], a command-line tool to interact with architecture diagrams written according to the [C4 model]. [`structurizr-cli`]: https://docs.structurizr.com/cli [C4 model]: https://c4model.com/ Co-authored-by: Tom van Dijk <[email protected]>
f144cd4 to
8dd338f
Compare
|
Thank you. |
Adds a new package structurizr-cli which is a command-line tool to interact with architecture diagrams written according to the C4 model.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.