Skip to content
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 support for owner filed in package manifest #536

Merged
merged 6 commits into from
Jun 23, 2020
Merged

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Jun 22, 2020

In some cases it is important to know who the owner of a package is so this owner can be pinged for questions. In #451 the different options around owner, authors, contributors were discussed. My take away from this discussions which lead to this PR:

  • There are multiple authors over time
  • Authors and contributors are mostly exchangeable terms
  • There can only be 1 owner, even if it is a team.

This PR currently only implements owner.github because it is all we need. Later on we can add an array of contributors and addition owner fields if needed.

Closing #451

@ruflin ruflin requested a review from ph June 22, 2020 14:13
@ruflin ruflin self-assigned this Jun 22, 2020
@ruflin
Copy link
Member Author

ruflin commented Jun 22, 2020

@andresrc @mtojek If we move forward with this, I suggest we update the packages to have the owner team on Github inside this field.

@ruflin ruflin added the Ingest Management:beta1 Group issues for ingest management beta1 label Jun 22, 2020
@elasticmachine
Copy link

elasticmachine commented Jun 22, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Started by upstream project "Beats/package-storage/master" build number 37]

  • Start Time: 2020-06-23T14:40:56.058+0000

  • Duration: 7 min 25 sec

Test stats 🧪

Test Results
Failed 0
Passed 127
Skipped 0
Total 127

@mtojek
Copy link
Contributor

mtojek commented Jun 23, 2020

To be honest, I'm against this change to prevent community people from pinging single developers instead of using team aliases. There is no plan for fixing this field if the person leaves the company and we won't fix already released packages.

I suggest to base on the repository owners here.

@ruflin
Copy link
Member Author

ruflin commented Jun 23, 2020

I used owner.github to exactly allow teams in here. For example owner.github: elastic/integrations-platforms.

@@ -28,6 +28,10 @@ license: basic
# The default type is integration and will be set if empty.
type: integration

# Details about the owner of the package. Current only github link is supported.
# A package can only have a single owner, the owner can be a user or a team.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtojek I added a comment here to make it more obvious it can be a user or a team.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. I assume that team aliases will be set for all Elastic packages?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I hope so. This was the initial driver for this in #451 Not sure if there is a good way to add this to the generator as in Filebeat for example, some belong to SIEM, some to Platforms team.

In some cases it is important to know who the owner of a package is so this owner can be pinged for questions. In elastic#451 the different options around owner, authors, contributors were discussed. My take away from this discussions which lead to this PR:

* There are multiple authors over time
* Authors and contributors are mostly exchangeable terms
* There can only be 1 owner, even if it is a team.

This PR currently only implements `owner.github` because it is all we need. Later on we can add an array of `contributors` and addition owner fields if needed.

Closing elastic#451
@ruflin
Copy link
Member Author

ruflin commented Jun 23, 2020

After going through a few merge conflicts and rebases, this should now be ready again @mtojek

CHANGELOG.md Outdated
@@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* Support multiple packages paths. [#525](https://github.com/elastic/package-registry/pull/525)
* Added support for ecs style validation for dataset fields. [#520](https://github.com/elastic/package-registry/pull/520)
* Use BasePackage for search output data. [#529](https://github.com/elastic/package-registry/pull/529)
* Add support for owner filed in package manifest. [#535](https://github.com/elastic/package-registry/pull/535)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: field

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 536

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. that is what happens if you try to predict the PR number ...

@ruflin ruflin merged commit 660008f into elastic:master Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ingest Management:beta1 Group issues for ingest management beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants