-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
feat: try to clean up and simplify the file adding interface #255
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/goreleaser/nfpm/mza41etdl |
This is a pretty significant change and should definitely be discussed. I do think your approach is cleaner and more elegant but the old one is probably more pragmatic, since most people probably have a whole bunch of regular files and rarely any special files. I have no data to back that up though. I generate my configs automatically as a part of a bigger custom build process, but for those writing configs by hand, it will be much more to type. I'll have a look at the exact changes when I find some time, maybe tomorrow. |
Ya, that's why I am making sure to ensure backwards compatibility so most people can continue to use the simple config format if they want. |
If I read your code correctly you planned to implement a custom unmarshal method so Maybe we can use Also I dislike the name of the type |
This is also nice for #43 because now we can then completely decide what do based on the YAML node type:
I think then |
00a8895
to
dc82afe
Compare
dc82afe
to
9f785b3
Compare
If we want a new field instead of trying to override the current contents:
- src: "something"
dst: "/opt/pkg/something"
type: "config"
packager: "rpm"
mode: 0644 give me a day or so to play with that concept to see if it turns out cleaner |
9f785b3
to
7e7b3f3
Compare
7e7b3f3
to
dc1c788
Compare
Much cleaner code now that I am not trying to mess with existing fields |
dc1c788
to
c238d91
Compare
c238d91
to
546d25d
Compare
546d25d
to
6d05821
Compare
6d05821
to
c858c29
Compare
Codecov Report
@@ Coverage Diff @@
## master #255 +/- ##
==========================================
+ Coverage 68.79% 70.38% +1.58%
==========================================
Files 9 9
Lines 1032 1138 +106
==========================================
+ Hits 710 801 +91
- Misses 190 209 +19
+ Partials 132 128 -4
Continue to review full report at Codecov.
|
…at for specifying files
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.
this looks awesome, thanks for working on this @djgilcrease ! 🚀
I would say we deprecate the old format. The new format is simple enough for the basic use case as well, and then when users need to change it, its not that much of a change? wdyt? if you agree, we can copy the deprecate package from goreleaser and do basically the same thing here, and remove the old option after 6mo or so. (and we can make this all in another PR since this one is already pretty big) |
Co-authored-by: Carlos Alexandro Becker <[email protected]>
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
The goal is to make this backward compatible with current config files so people can stay with the simplified format until they need more advanced features.
Internally all packagers
ONLY
use the Contents data which is a merging of all the old file specifications into one list. IF a file was specified in a packager specific override (egrpm.Files
) then thecontent.Packager
field is set to the correct packager automatically which ensures files for other packagers do not get mixed together.Example
The
files
,config_files
,symlinks
,rpm.config_noreplace_files
, andrpm.ghost_files
are merged intocontents
and all packagers only the the data fromcontents
now. This makes it simple for people to leave their config files as is and not break anything, but as they need more advanced features, like control over the file mode, owner, group, etc they can convert all their files to the contents as well.I have also set it up so the simple mapping will work as well
Also this is setup to be ready to handle contents as a string in prep for #43 but is not implemented