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

Source Jira: Add pull requests stream #7378

Closed
wants to merge 8 commits into from

Conversation

cjwooo
Copy link
Contributor

@cjwooo cjwooo commented Oct 26, 2021

What

Adds a Pull Requests stream to fetch data on linked pull requests for Jira issues. Currently only supports GitHub pull requests.
Also fixes the field_ids_by_name() method on the IssueFields stream to handle multiple field ids per field name, a case we've seen with some of our projects.

How

Uses an undocumented Jira API to fetch data on linked pull requests. Only runs on Jira issues that have been determined to have linked pull requests via their "Development" field.

Recommended reading order

  1. x.java
  2. y.python

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

  • Community member? Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • docs/SUMMARY.md
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • PR name follows PR naming conventions
  • Connector added to connector index like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here

Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • PR name follows PR naming conventions
  • Connector version bumped like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here

Connector Generator

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
  • Documentation which references the generator is updated as needed.

@cjwooo
Copy link
Contributor Author

cjwooo commented Oct 26, 2021

Ping @marcosmarxm

@github-actions github-actions bot added the area/connectors Connector related issues label Oct 26, 2021
Comment on lines 779 to 788
class PullRequests(IncrementalJiraStream):
"""
Undocumented internal API used by Jira webapp
"""

cursor_field = "updated"
parse_response_root = "detail"
# raise_on_http_errors = False

pr_regex = r"(?P<prDetails>PullRequestOverallDetails{openCount=(?P<open>[0-9]+), mergedCount=(?P<merged>[0-9]+), declinedCount=(?P<declined>[0-9]+)})|(?P<pr>pullrequest={dataType=pullrequest, state=(?P<state>[a-zA-Z]+), stateCount=(?P<count>[0-9]+)})"
Copy link
Member

Choose a reason for hiding this comment

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

I don't feel comfortable adding this endpoint. If this starts failing we cann't solve because its not possible to know where to find information to solve, backoff or error handling. @cjwooo can you explain your use case to use this endpoint? Maybe let to add this after Jira allow in official API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use this to associate closed Jira issues with the Github pull requests that resolved the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it help if I mark this stream as an experimental one in the comments?

Copy link
Member

Choose a reason for hiding this comment

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

@sherifnada can you give your opinion here?

@marcosmarxm marcosmarxm self-assigned this Nov 8, 2021
@sherifnada
Copy link
Contributor

@cjwooo thanks for opening this PR! I took a look around online and it while this endpoints seems(?) to maybe have held up via Hyrum's law, it's difficult to support adding it to the connector because it's undocumented; if it changes or breaks or gets removed at any point, it'll cause user disruption. We could always say something like "well, blame JIRA because they removed it" but the problem will likely take the shape of undocumented/unannounced changes which cause ongoing maintenance work on this connector. How have you been thinking about this issue in the context of your product?

@cjwooo
Copy link
Contributor Author

cjwooo commented Nov 9, 2021

@sherifnada If this API breaks suddenly, we currently inform the user, via sync logs, that we can't fetch linked pull requests, and continue pulling the rest of our equivalent of Airbyte streams normally. I can implement a similar strategy here, where the Pull Requests stream will log that the stream is broken due to changes in the Jira API, without failing the entire source sync.

@cjwooo
Copy link
Contributor Author

cjwooo commented Nov 15, 2021

@sherifnada Would that be acceptable?

@marcosmarxm
Copy link
Member

(waiting updates from user adding safeguards)

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Dec 3, 2021
@cjwooo
Copy link
Contributor Author

cjwooo commented Dec 3, 2021

@marcosmarxm This is ready for review.

@marcosmarxm
Copy link
Member

Thanks @cjwooo

@cjwooo
Copy link
Contributor Author

cjwooo commented Dec 16, 2021

Bump

@marcosmarxm
Copy link
Member

marcosmarxm commented Dec 21, 2021

@cjwooo we're waiting the solution for #9017 to publish your contribution. Sorry the long delay.

Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

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

sorry the long delay @cjwooo

@marcosmarxm
Copy link
Member

@cjwooo could you give me permission to your repo? Or solve the conflicts so I can merge your contribution? If helps please see my PR: #8559

return f"https://{self._domain}/rest/dev-status/1.0/"

def path(self, **kwargs) -> str:
return f"issue/detail"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return f"issue/detail"
return "issue/detail"

remove f-string is breaking the format

@marcosmarxm
Copy link
Member

The connector was published in #8559 I'm closing this PR and merging the other to relese the new version of the connector.

@cjwooo cjwooo deleted the cwu/jiraprs branch May 18, 2022 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants