Add handling for docs files and license files #256
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Jeremy Rickard <jrickard@microsoft.com>
12 tasks
Signed-off-by: Jeremy Rickard <jrickard@microsoft.com>
jeremyrickard
changed the title
DannyBrito
reviewed
May 17, 2024
Signed-off-by: Jeremy Rickard <jrickard@microsoft.com>
cpuguy83
requested changes
May 17, 2024
spec.go
Outdated
| @@ -131,6 +131,10 @@ type Artifacts struct { | |||
| Directories *CreateArtifactDirectories `yaml:"createDirectories,omitempty" json:"createDirectories,omitempty"` | |||
| // ConfigFiles is a list of files that should be marked as config files in the package. | |||
| ConfigFiles map[string]ArtifactConfig `yaml:"configFiles,omitempty" json:"configFiles,omitempty"` | |||
| // DocFiles is a list of doc files included in the package | |||
| DocFiles []string `yaml:"docFiles,omitempty" json:"docFiles,omitempty"` | |||
There was a problem hiding this comment.
The reason I like to make these structs is so that it is not a breaking change to add additional options.
I also think subdir is a thing relavent for these files (and licenses).
There was a problem hiding this comment.
Got it to work. I needed to handle the copy myself for the subpaths and emit the full path inside the %files section.
spec.go
Outdated
| @@ -131,6 +131,10 @@ type Artifacts struct { | |||
| Directories *CreateArtifactDirectories `yaml:"createDirectories,omitempty" json:"createDirectories,omitempty"` | |||
| // ConfigFiles is a list of files that should be marked as config files in the package. | |||
| ConfigFiles map[string]ArtifactConfig `yaml:"configFiles,omitempty" json:"configFiles,omitempty"` | |||
| // DocFiles is a list of doc files included in the package | |||
| DocFiles []string `yaml:"docFiles,omitempty" json:"docFiles,omitempty"` | |||
There was a problem hiding this comment.
Also can we name these docs and licenses in the yaml/json?
There was a problem hiding this comment.
Yep, I updated. I also changed the struct fields to Docs and Licenses, that seemed like better naming to me.
jeremyrickard
force-pushed
the
add-doc-dir
branch
from
7c0b1e7
to
85a897c
Compare
Signed-off-by: Jeremy Rickard <jrickard@microsoft.com>
jeremyrickard
force-pushed
the
add-doc-dir
branch
from
85a897c
to
0b8cc0b
Compare
cpuguy83
reviewed
May 17, 2024
spec.go
Outdated
| @@ -131,6 +131,10 @@ type Artifacts struct { | |||
| Directories *CreateArtifactDirectories `yaml:"createDirectories,omitempty" json:"createDirectories,omitempty"` | |||
| // ConfigFiles is a list of files that should be marked as config files in the package. | |||
| ConfigFiles map[string]ArtifactConfig `yaml:"configFiles,omitempty" json:"configFiles,omitempty"` | |||
| // DocFiles is a list of doc files included in the package | |||
There was a problem hiding this comment.
These doc strings are not correct anymore 🙈
There was a problem hiding this comment.
Suggested change
| // DocFiles is a list of doc files included in the package | |
| // Docs is a list of doc files included in the package |
cpuguy83
reviewed
May 17, 2024
spec.go
Outdated
| @@ -131,6 +131,10 @@ type Artifacts struct { | |||
| Directories *CreateArtifactDirectories `yaml:"createDirectories,omitempty" json:"createDirectories,omitempty"` | |||
| // ConfigFiles is a list of files that should be marked as config files in the package. | |||
| ConfigFiles map[string]ArtifactConfig `yaml:"configFiles,omitempty" json:"configFiles,omitempty"` | |||
| // DocFiles is a list of doc files included in the package | |||
| Docs map[string]ArtifactConfig `yaml:"docs,omitempty" json:"docs,omitempty"` | |||
| // LicenseFiles is a list of doc files included in the package | |||
There was a problem hiding this comment.
Suggested change
| // LicenseFiles is a list of doc files included in the package | |
| // Licenses is a list of doc files included in the package |
Signed-off-by: Jeremy Rickard <jrickard@microsoft.com>
cpuguy83
approved these changes
May 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This PR adds the ability to add files to the package and have them represented by the
%docdirective or the %license directive within the %files list.There isn't a corresponding install entry, because RPM handles this already:
The same happens with license files.
I thought that a slice of strings made more sense than using ArtifactConfig here, since we don't really need to provide any other real options. It seems like you can specify a full path with the %doc directive (%license as well), but I think we can go with this for now? It feels cleaner/more organized.
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):fixes #241
Special notes for your reviewer: