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

Introduce create ticket script #1

Merged
merged 1 commit into from
Dec 1, 2021
Merged

Introduce create ticket script #1

merged 1 commit into from
Dec 1, 2021

Conversation

oshoval
Copy link
Owner

@oshoval oshoval commented Nov 4, 2021

The script automates github issue to Jita ticket mirroring.

Copy link
Collaborator

@EdDev EdDev left a comment

Choose a reason for hiding this comment

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

I am unclear which tests are written to cover all the code. Are these the main functions?
I suggest you create a tests folder` and create there test modules using pytest that import the production code.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
LICENSE Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
githublib.py Outdated Show resolved Hide resolved
githublib.py Outdated Show resolved Hide resolved
githublib.py Outdated Show resolved Hide resolved
githublib.py Outdated Show resolved Hide resolved
jiralib.py Outdated Show resolved Hide resolved
@EdDev
Copy link
Collaborator

EdDev commented Nov 10, 2021

Please note that for deployment, it is recommended to use a setup file: https://docs.python.org/3/distutils/setupscript.html

If this is for internal use only, then you do not need it.
If you want to maintain it for others to use, you need it.

@oshoval
Copy link
Owner Author

oshoval commented Nov 10, 2021

Please note that for deployment, it is recommended to use a setup file: https://docs.python.org/3/distutils/setupscript.html

If this is for internal use only, then you do not need it. If you want to maintain it for others to use, you need it.

Thanks
For now we start with internal use.

@oshoval
Copy link
Owner Author

oshoval commented Nov 10, 2021

Thanks

addressed comments,
didn't add yet tests, most of them are pretty tricky because the github status changes (unless we look always on all the history),
and we can't create issues on Jira in tests (we can just check for example a known issue exists).

new files
config.py
ticket.py
main.py
removed the common.py

maybe worth to remove github from Ticket class, and just send repo to it, since its the only thing it uses

@oshoval oshoval requested a review from EdDev November 10, 2021 13:59
@oshoval oshoval changed the title Introduce create_ticket script Introduce create ticket script Nov 10, 2021
@oshoval
Copy link
Owner Author

oshoval commented Nov 10, 2021

https://github.com/oshoval/github2jira/compare/25d5236..b717bdf
I think this should include all the pushes (some of them hidden in others, and some are small)

@oshoval
Copy link
Owner Author

oshoval commented Nov 11, 2021

Refactor the issue class

@oshoval
Copy link
Owner Author

oshoval commented Nov 11, 2021

Added unit tests for Issue class

@oshoval
Copy link
Owner Author

oshoval commented Nov 24, 2021

addressed rest of the comments

github2jira/jiralib.py Show resolved Hide resolved
github2jira/jiralib.py Outdated Show resolved Hide resolved
@oshoval
Copy link
Owner Author

oshoval commented Nov 25, 2021

addressed comment

@oshoval
Copy link
Owner Author

oshoval commented Nov 25, 2021

added some more tests, and tests/common lib

Copy link
Collaborator

@EdDev EdDev left a comment

Choose a reason for hiding this comment

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

The production code seems good, I only had small comments about it, nothing major.

The unit tests require more work.
Although I will not insist on it, please note that the unit tests are still suppose to check scenarios and not individual methods. I mean:

  • checking that you can filter the relevant github issues.
  • Opening a Jira ticket, based on some data.
  • Failing to open a Jira ticket or contacting github.
  • Not finding any relevant issues on github to process.

Etc...

github2jira/githublib.py Outdated Show resolved Hide resolved
github2jira/ticketmanager.py Outdated Show resolved Hide resolved
test_ticketmanager.py Outdated Show resolved Hide resolved
tests/common.py Outdated Show resolved Hide resolved
tests/common.py Outdated Show resolved Hide resolved
tests/test_jiralib.py Outdated Show resolved Hide resolved
tests/test_jiralib.py Outdated Show resolved Hide resolved
tests/test_jiralib.py Outdated Show resolved Hide resolved
tests/test_github.py Outdated Show resolved Hide resolved
tests/test_jiralib.py Outdated Show resolved Hide resolved
@oshoval
Copy link
Owner Author

oshoval commented Nov 29, 2021

The production code seems good, I only had small comments about it, nothing major.

The unit tests require more work. Although I will not insist on it, please note that the unit tests are still suppose to check scenarios and not individual methods. I mean:

  • checking that you can filter the relevant github issues.
  • Opening a Jira ticket, based on some data.
  • Failing to open a Jira ticket or contacting github.
  • Not finding any relevant issues on github to process.

Etc...

Thanks,
some of those sound as e2e,
I prefer to not develop this project any deeper right now in aspect of tests, and provide as is.
In case we decide we can improve it later.

@oshoval
Copy link
Owner Author

oshoval commented Nov 29, 2021

addressed most of the comments

@oshoval
Copy link
Owner Author

oshoval commented Nov 29, 2021

Thanks, addressed rest of the comments (unless unresolved)

@oshoval
Copy link
Owner Author

oshoval commented Nov 30, 2021

since some Jira servers will deprecate basic auth and move to PAT, support it

Copy link
Collaborator

@EdDev EdDev left a comment

Choose a reason for hiding this comment

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

some of those sound as e2e,

I do not think they are, as the intention is to unit test scenarios that have meaning (if possible).

I prefer to not develop this project any deeper right now in aspect of tests, and provide as is.
In case we decide we can improve it later.

Not investing in tests is raising the risk you still have bugs or worse, that you may introduce bugs in the future.
As you are the one who needs to maintain this, it is your burden to carry so I will not push you to do anything beyond what you want.

github2jira/githublib.py Outdated Show resolved Hide resolved
github2jira/ticketmanager.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
test_ticketmanager.py Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
tests/test_github.py Outdated Show resolved Hide resolved
tests/test_github.py Outdated Show resolved Hide resolved
@oshoval
Copy link
Owner Author

oshoval commented Dec 1, 2021

I do not think they are, as the intention is to unit test scenarios that have meaning (if possible).

Some aren't possible.

For example jira.create_issue, i already checked the jira create data but
beside this, the only thing the function does is call the real Jira lib, which we can't do in unit test.
the real jira function even doesn't return a value (just assert in case of error, and i don't try to catch it anyhow even on main)

Anyhow as we said, I prefer to first publish an initial version, and upon needs to develop it,
maintaining of it is very easy, and i don't see we need to invest it any deeper right now.

The script automates github issue to Jita ticket mirroring.

Signed-off-by: Or Shoval <oshoval@redhat.com>
@oshoval
Copy link
Owner Author

oshoval commented Dec 1, 2021

thanks, addressed comments

Copy link
Collaborator

@EdDev EdDev left a comment

Choose a reason for hiding this comment

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

Nice work!

@oshoval
Copy link
Owner Author

oshoval commented Dec 1, 2021

Thanks for the great review Edy

@oshoval oshoval merged commit 676bd33 into main Dec 1, 2021
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.

2 participants