Skip to content

Conversation

@Adityamalik123
Copy link
Contributor

@Adityamalik123 Adityamalik123 commented Nov 12, 2022

The atlassian-jira provider currently uses JIRA> 1.0.7 dependency which is an unofficial Jira SDK, We need to replace it with the official SDK atlassian-python-api to allow more atlassian product operators in Airflow.
closes : #25669

@Adityamalik123 Adityamalik123 marked this pull request as ready for review November 12, 2022 12:12
@eladkal eladkal self-requested a review November 13, 2022 08:03
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

Lets also add entry in the provider change log explaining this breaking change

Comment on lines 90 to 91
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to catch the exceptions at all?
It will just raise whatever exception coming from the sdk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the changes here by only keeping the HTTPError to log the response from it. Other will just raise with whatever exception that comes. Thanks for identifying this.

Comment on lines 79 to 88
Copy link
Contributor

Choose a reason for hiding this comment

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

If we remove the notes since no longer relevant shouldn't the code change as well?
to my understanding this code is a workaround to address the issue in comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The official SDK returns 'id' in the output in all jira issue related methods. I have added these https://github.com/apache/airflow/pull/27633/files#diff-85731758409b94fa7ff2ab57860f1d4242f7a196768f003e7b618d88254afba4R84-R85 which takes id from the response and performs xcom_push with that.

Comment on lines 88 to 86
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment/question

Copy link
Contributor

Choose a reason for hiding this comment

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

same question

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the changes here by only keeping the HTTPError to log the response from it. Other will just raise with whatever exception that comes.
Ref: https://github.com/apache/airflow/pull/27633/files#diff-8edbc4ba86e754ffa8636bf167fa0a0e1eb9bd47f134a1d78103b3b7dad55b10R130

Comment on lines 68 to 80
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't really need this warnings?
we are changing SDKs.. parameters of the previous SDK can just cause confusion.
If users didn't change their usage of the hook nothing won't work anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I have removed these warnings.

@potiuk
Copy link
Member

potiuk commented Nov 27, 2022

Can we count on rebase/fixes @Adityamalik123 ?

@Adityamalik123
Copy link
Contributor Author

@potiuk @eladkal Apologies for the delay, I'll be pushing up the fixes this week.

@Adityamalik123
Copy link
Contributor Author

Lets also add entry in the provider change log explaining this breaking change

@eladkal Can i add it in the CHANGELOG.rst under a new version or do i have to comment it here and the release manager adds it?

@eladkal
Copy link
Contributor

eladkal commented Dec 1, 2022

Lets also add entry in the provider change log explaining this breaking change

@eladkal Can i add it in the CHANGELOG.rst under a new version or do i have to comment it here and the release manager adds it?

Yeah. You need to add it. Check opsgenie change log for example as it had similar case of changing sdk

@Adityamalik123
Copy link
Contributor Author

Adityamalik123 commented Dec 4, 2022

Lets also add entry in the provider change log explaining this breaking change

@eladkal Can i add it in the CHANGELOG.rst under a new version or do i have to comment it here and the release manager adds it?

Yeah. You need to add it. Check opsgenie change log for example as it had similar case of changing sdk

Thanks! I have made the changes. Please re-review.

Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

LGTM
left one comment

@potiuk
Copy link
Member

potiuk commented Dec 5, 2022

Can you also please update provider.yaml and add 2.0.0 version there?

@Adityamalik123
Copy link
Contributor Author

Can you also please update provider.yaml and add 2.0.0 version there?

Sure, I have pushed the same.

@eladkal eladkal merged commit f3c68d7 into apache:main Dec 7, 2022
@eladkal
Copy link
Contributor

eladkal commented Dec 7, 2022

Thanks @Adityamalik123
Now it should be easy to add new integrations of atlassian products (service desk or others)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

change Jira sdk to official atlassian sdk

4 participants