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

changed test_requirement.txt removed one line #122

Merged
5 commits merged into from
Mar 3, 2020

Conversation

ANA-POTJE
Copy link
Contributor

@ANA-POTJE ANA-POTJE commented Feb 26, 2020

Description

Fixing the Github issue #119 Remove trufflehog pinned dependency in test_requirements.txt

Development notes

QA notes

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Legal notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

@ANA-POTJE ANA-POTJE requested a review from 921kiyo as a code owner February 26, 2020 20:10
@921kiyo 921kiyo self-assigned this Feb 28, 2020
Copy link
Contributor

@921kiyo 921kiyo left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. It looks good to me 👍

  • We would like to renogise your name in the release note, so could you please update RELEASE.md with your name and Github link please? So I will merge it once it is addressed.

  • Also it is a good practice to add the description of which Github issue you are addressing in the PR ([KED-1404] Remove trufflehog pinned dependency in test_requirements.txt #119 in this case). So it would be great if you could update the PR description.

  • Could you please click the checkbox in the PR (especially legal note)

Otherwise, great work and thank you for your contribution :)

@@ -10,4 +10,3 @@ requests-mock>=1.6.0, <2.0
flake8>=3.5, <4.0
isort
trufflehog>=2.0.99, <3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also update the lower bound of trufflehog to be
trufflehog>=2.1.0, <3.0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Kiyo! Some questions ...
(1) " .. update RELEASE.md with your name and Github link please? .."
I believe I should change RELEASE.md, then again: "git commit / git push / create a new PR, am I correct?

(2) " ... add the description of which Github issue you are addressing in the PR ..."
Done! Please check ...

(3) " .. Could you also update the lower bound of trufflehog to be trufflehog>=2.1.0, <3.0 ? .."
Is this a new request, or it is inside the same Github issue?
Should I update RELEASE.md, update the lower bound of trufflehog and then "git commit / git push ..."?

Copy link
Contributor Author

@ANA-POTJE ANA-POTJE Feb 28, 2020

Choose a reason for hiding this comment

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

Sorry, one more question … Where exactly in the RELEASE.md file should I include my name and GitHub link please? Should I include a section in the beginning of the file such as:
' ## supporting contributions
' Ana Potje
Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@ANA-POTJE
You don't need to create a new PR. You can update RELEASE.md and lower bound of trufflehog (I suggested the change below, which you can accept), then "git commit/ git push" in this PR.

For the RELEASE.md, see how the previous contributors added their name in https://github.com/quantumblacklabs/kedro/blob/develop/RELEASE.md

Let me know if anything is not clear :)

Copy link
Contributor

Choose a reason for hiding this comment

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

For FYI, see how the person updated the RELEASE.md in a PR. https://github.com/quantumblacklabs/kedro/pull/93/files

package/test_requirements.txt Outdated Show resolved Hide resolved
ANA-POTJE and others added 2 commits March 3, 2020 14:13
Co-Authored-By: Kiyohito Kunii (Kiyo) <8097799+921kiyo@users.noreply.github.com>
@ANA-POTJE
Copy link
Contributor Author

Hi there, changes were done (included my name in RELEASE.md updated the lower bound of trufflehog to be trufflehog>=2.1.0, <3.0, as requested.
I managed to run a git commit successfully.
But I got an error in my git push … possibly because my local version cloned from kedro-viz is outdated (?), would you please kindly guide me? Thanks!!!

@ghost
Copy link

ghost commented Mar 3, 2020

Hey @ANA-POTJE, thanks for this. It all seems fine here, so your git push seems to have worked. If you're getting an error, you might try git pull before git push but it's hard to say without the error.

Could you also please tick the legal notice box in the PR so we can merge your PR in? :)

@ANA-POTJE
Copy link
Contributor Author

Hi Zain, "legal notice" box is ticked … I hope all ready for merge now? ;)

@ghost ghost merged commit 9d20abd into kedro-org:develop Mar 3, 2020
@ghost
Copy link

ghost commented Mar 3, 2020

Merged. :)

@ANA-POTJE
Copy link
Contributor Author

That's great!!! Very happy to help! :)))

This pull request was closed.
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