-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add NOTICE.txt file at the root level of a package #151
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Trends 🧪 |
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.
LGTM. But lets way on a 👍 from @ycombinator or @mtojek .
@rw-access As soon as we get it in, could you follow up with an issue in Kibana to make sure we also add the logic in Kibana to expose it. Otherwise it will be pretty hard for any user to find it.
@@ -19,6 +19,11 @@ spec: | |||
name: "changelog.yml" | |||
required: true | |||
$ref: "./changelog.spec.yml" | |||
- description: The package's NOTICE file | |||
type: file | |||
contentMediaType: "text/plain" |
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.
Is it necessary to define the content type here provided that no action is taken based on this? Did you try without this definition?
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.
I just made an assumption that this was necessary. I'm not intimately familiar with the details but am glad to remove if that is preferred.
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.
I'm okay leaving this in. We may not take any action on this content type today but if we do in the future, the NOTICE file would automatically get covered.
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.
LGTM.
Thanks @ycombinator and @ruflin! |
What does this PR do?
Closes #149
Adds a new optional NOTICE.txt file to a package. This isn't an asset but is the NOTICE.txt for the whole package.
Why is it important?
Some assets can be derived from licensed material and require us to propogate NOTICE.txt. The current use-case is for the
security_rule
asset type, because some rules (basically query + metadata) have been derived from license materials.We will need a way to present this information to the user. I imagine the specific integration page is sufficient.
Checklist
test/packages
that prove my change is effective. I added an asset to the "good" packageversions/N/changelog.yml
.Related issues
#142 (PR)
elastic/package-storage#843 (PR)