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

[enriched] Add support for github pull request data #399

Merged
merged 2 commits into from
Jul 10, 2018

Conversation

valeriocos
Copy link
Member

@valeriocos valeriocos commented Jul 8, 2018

The main aim of this PR is to trigger the discussion about how to integrate GitHub pull request data in GrimoireLab. This PR proposes to extend GitHubOcean and GitHubEnrich to include the new data without creating a new connector. Conversely, PR #398 proposes to add a new connector to ELK.

This PR requires a modification to mordred (PR chaoss/grimoirelab-sirmordred#175) in order to allow the definition of multiple sections for the same backend (thus enabling the explotation of backends with multiple categories in a transparent way). On the other hand, PR #398 doesn't need to modify mordred, however it implies the creation of a dedicated connector for each category of a backend.

Given that both solutions have advantages and drawbacks, what are your thoughts @acs @jgbarah @aswanipranjal ?

This code proposes a possible implementation to include support
for github pull request data in ELK.
This PR extends the mappings for the GitHub raw data to enable
the support for pull request data,
@aswanipranjal
Copy link
Contributor

@valeriocos I think this is much better than what I propose.

Copy link
Member

@acs acs left a comment

Choose a reason for hiding this comment

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

LGTM

@valeriocos
Copy link
Member Author

If everybody agrees on this structure, @aswanipranjal could you modify your PR #398 according to what is proposed in this PR?
Thank you!

@jgbarah
Copy link
Contributor

jgbarah commented Jul 9, 2018

Sorry for coming late to the discussion, but I agree with you all.

What I don't understand is if you propose to substitute this pr with #398 (once modified), or to follow on until this one is merged.

@aswanipranjal
Copy link
Contributor

@valeriocos @jgbarah We should merge this PR and then I can create a commit to calculate the rich PR data

@valeriocos
Copy link
Member Author

sorry for not being clear @jgbarah . I agree with @aswanipranjal on merging this PR and then creating a new one with the rich PR data.

@aswanipranjal
Copy link
Contributor

@acs, please merge this PR?

@acs acs merged commit 8369a57 into chaoss:master Jul 10, 2018
@acs
Copy link
Member

acs commented Jul 10, 2018

Done! Thanks for the heads up @aswanipranjal !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants