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

rename to tirith #76

Merged
merged 4 commits into from
Oct 4, 2022
Merged

rename to tirith #76

merged 4 commits into from
Oct 4, 2022

Conversation

Akshat0694
Copy link
Member

No description provided.

Copy link
Member

@refeed refeed left a comment

Choose a reason for hiding this comment

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

Some few small suggestions, otherwise, at a glance, looks good for me

Also don't forget to comply to Black before merging the code ;)

@@ -114,4 +114,4 @@ def pretty_print_result_dict(final_result_dict: Dict) -> None:
elif final_result_dict["final_result"] is None:
print(TermStyle.skipped("= Skipped final evaluator"))
else:
print(TermStyle.fail("✘ Not passed final evaluator"))
print(TermStyle.fail("✘ Failed final evaluation"))
Copy link
Member

Choose a reason for hiding this comment

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

I think the passed and skipped messages should also be changed to use the term "evaluation" instead of "evaluator"

"Changelog": "https://github.com/stackguardian/policy-framework/blob/main/CHANGELOG.md",
"Issue Tracker": "https://github.com/stackguardian/policy-framework/issues",
"Changelog": "https://github.com/stackguardian/tirith/blob/main/CHANGELOG.md",
"Issue Tracker": "https://github.com/stackguardian/tirith/issues",
},
keywords=["iac", "policy", "terraform", "policy as code"],
python_requires=">=3.8.*",
Copy link
Member

Choose a reason for hiding this comment

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

I think we also need to renew the install_requires to match Pipfile content, e.g. if people were to install this using pip install tirith (assumming we will use the name tirith in pypi) it will only install the deps listed in install_requires in this setup.py, not from the pipfile. Good way to test this is to create a new venv and do pip install . to see what it installs

A good tool to automate this is pypa/pipenv#1263 (comment)

Otherwise just writing it down manually is OK as well because our Pipfile currently only has 2 deps

Copy link
Member

Choose a reason for hiding this comment

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

This can also be done in a separate PR though

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 4, 2022

Please retry analysis of this Pull-Request directly on SonarCloud.

@Akshat0694 Akshat0694 merged commit 4bb87ff into main Oct 4, 2022
@Akshat0694 Akshat0694 deleted the f/change_name_to_tirith branch October 4, 2022 20:11
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