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

Update graal.py to make it compatible with non-public repos #1114

Merged

Conversation

GrayStranger
Copy link
Contributor

Anonymize raw origin and tag url to make it more universal for cocom\colic etc. backends and to make it compatible with non-public repos.

Copy link
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

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

First of all, thanks for the patch. I added some minor comments you need to address before we merge the PR.
For the comment of the commit, I would replace it by:

[graal] Anonymize repository urls

Anonymize raw origin and tag url to make it more universal for
cocom\colic, etc. metrics and to make it compatible with non-public
repos.

Signed-off-by: GrayStranger <[email protected]>

We also require to sign off the commit but the bot is failing for some reason. Maybe the format?

grimoire_elk/raw/graal.py Outdated Show resolved Hide resolved
grimoire_elk/raw/graal.py Show resolved Hide resolved
Copy link
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. Apparently now there are some format errors:

Run poetry run flake8
./grimoire_elk/raw/graal.py:20:2: W291 trailing whitespace
./grimoire_elk/raw/graal.py:27:1: E302 expected 2 blank lines, found 1
./grimoire_elk/raw/graal.py:70:1: W293 blank line contains whitespace
Error: Process completed with exit code 1.

@GrayStranger
Copy link
Contributor Author

Thanks for the fix. Apparently now there are some format errors:

Run poetry run flake8
./grimoire_elk/raw/graal.py:20:2: W291 trailing whitespace
./grimoire_elk/raw/graal.py:27:1: E302 expected 2 blank lines, found 1
./grimoire_elk/raw/graal.py:70:1: W293 blank line contains whitespace
Error: Process completed with exit code 1.

fixed

sorry, i have no a lot of experience with GitHub contributing :). Hope now it should works.

Copy link
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

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

The PR looks good to me. Before we merge it, would you mind to squash all commits into one? If you can't do it, I can do it myself.

Btw, test are failing because the last version of poetry doesn't support Python 3.7. We're fixing it.

@GrayStranger GrayStranger force-pushed the GrayStranger-patch-rawurl-anonymize branch 4 times, most recently from 633a671 to 5a0e38d Compare August 23, 2023 16:32
@GrayStranger
Copy link
Contributor Author

The PR looks good to me. Before we merge it, would you mind to squash all commits into one? If you can't do it, I can do it myself.

Btw, test are failing because the last version of poetry doesn't support Python 3.7. We're fixing it.

done

@GrayStranger GrayStranger force-pushed the GrayStranger-patch-rawurl-anonymize branch 2 times, most recently from 7e1c9be to 4cbfb99 Compare August 23, 2023 16:42
@GrayStranger
Copy link
Contributor Author

fixed signature in commit after sqashing

@GrayStranger
Copy link
Contributor Author

@sduenas I think now its all good to let it in, right? :)

sorry for ping.

@sduenas sduenas force-pushed the GrayStranger-patch-rawurl-anonymize branch from 4cbfb99 to a9266ec Compare August 29, 2023 22:40
@sduenas
Copy link
Member

sduenas commented Aug 30, 2023

@sduenas I think now its all good to let it in, right? :)

sorry for ping.

There's still some style errors:

./grimoire_elk/raw/graal.py:72:1: W293 blank line contains whitespace
Error: Process completed with exit code 1.

@GrayStranger GrayStranger force-pushed the GrayStranger-patch-rawurl-anonymize branch from a9266ec to a2edffc Compare August 30, 2023 12:49
@GrayStranger
Copy link
Contributor Author

whitespaces removed

@GrayStranger
Copy link
Contributor Author

could you please advice me - how can I perform such checks by myself?

@sduenas
Copy link
Member

sduenas commented Aug 30, 2023

could you please advice me - how can I perform such checks by myself?

If you do an online editing of the file I don't really know, but you can always download the file and run the python tool "flake8". The tests use this tool to see if it complies with the PEP8 specification (it's a guide of style for Python).

@GrayStranger GrayStranger force-pushed the GrayStranger-patch-rawurl-anonymize branch from a2edffc to 3ff2366 Compare August 30, 2023 15:04
@coveralls
Copy link

coveralls commented Aug 30, 2023

Pull Request Test Coverage Report for Build 6468834462

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 85.549%

Totals Coverage Status
Change from base Build 6467374446: 0.003%
Covered Lines: 7489
Relevant Lines: 8754

💛 - Coveralls

@GrayStranger
Copy link
Contributor Author

@sduenas just friendly reminder to merge it for the next release :)

Anonymize raw origin and tag url to make it more universal for
cocom\colic, etc. metrics and to make it compatible with non-public
repos.

Signed-off-by: GrayStranger <[email protected]>
@sduenas sduenas force-pushed the GrayStranger-patch-rawurl-anonymize branch from 3ff2366 to 7693537 Compare October 10, 2023 11:48
Copy link
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

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

LGTM

@sduenas sduenas merged commit 9369ce5 into chaoss:master Oct 10, 2023
7 of 8 checks passed
@sduenas
Copy link
Member

sduenas commented Oct 10, 2023

@GrayStranger merged.

I updated your branch to include a changelog file that we use for automating realeases and for rebasing to the latest commit. I hope you don't mind.

Thanks for your contribution.

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.

3 participants