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.

Sign up for GitHub

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

Jump to bottom

Fix #12436 - Migrate to pyproject.toml #14025

Merged
merged 13 commits into from
Nov 22, 2023
Merged

Fix #12436 - Migrate to pyproject.toml #14025

merged 13 commits into from
Nov 22, 2023

Conversation

pmbrull
Copy link
Collaborator

@pmbrull pmbrull commented Nov 19, 2023 •
edited
Loading

Describe your changes:

Fixes #12436

Moved all config files into pyproject.toml: coverage, pylint, setup.cfg and most of setup.py

since we can still use setuptools as the backend and in setup.py we have more liberty to handle dependencies programmatically, kept that info there to not repeat ourselves too much if we need to display everything statically in the TOML file

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

pmbrull
pmbrull commented Nov 19, 2023
View reviewed changes
.github/workflows/openmetadata-airflow-apis.yml
@@ -34,6 +34,6 @@ jobs:
run: |
make install_dev install_apis
cd openmetadata-airflow-apis; \
python setup.py build sdist bdist_wheel; \
python -m build; \
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the main piece that was going to be deprecated

@github-actions github-actions bot added the safe to test Add this label to run secure Github workflows on PRs label Nov 19, 2023
pmbrull
pmbrull commented Nov 19, 2023
View reviewed changes
Makefile
@@ -24,40 +24,6 @@ yarn_install_cache: ## Use Yarn to install UI dependencies
yarn_start_dev_ui: ## Run the UI locally with Yarn
cd openmetadata-ui/src/main/resources/ui && yarn start

## Ingestion Core
.PHONY: core_install_dev
core_install_dev: ## Prepare a venv for the ingestion-core module
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cleaning up the ingestion-core since we're not really making any use of it. We can recover the files if we have the need in the future

@github-actions GitHub Actions
Copy link
Contributor

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

@pmbrull pmbrull marked this pull request as ready for review November 19, 2023 18:48
@pmbrull pmbrull requested review from akash-jain-10, harshach and a team as code owners November 19, 2023 18:48
akash-jain-10
akash-jain-10 previously approved these changes Nov 20, 2023
View reviewed changes
Copy link
Collaborator

@akash-jain-10 akash-jain-10 left a comment

Choose a reason for hiding this comment

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

Thanks 👍🏼 . LGTM. Can you also update the Developer Docs required with this change ?

@pmbrull
Copy link
Collaborator Author

pmbrull commented Nov 20, 2023

Thanks 👍🏼 . LGTM. Can you also update the Developer Docs required with this change ?

Nothing needs to be updated. All the make commands remain the same. The only dev aspect updates was on the publishing github actions, the rest should be transparent

pmbrull
pmbrull commented Nov 20, 2023
View reviewed changes
Makefile
@@ -4,7 +4,7 @@ include ingestion/Makefile

.PHONY: help
help:
@grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[35m%-30s\033[0m %s\n", $$1, $$2}'
@grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | sort | awk 'BEGIN {FS = ":"}; {printf "\033[35m%-35s\033[0m %s\n", $$2, $$3}'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to fix this. The changes from #13677 left the help command sending the wrong info

@sonarqubecloud SonarQubeCloud
Copy link

[open-metadata-airflow-apis] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@akash-jain-10
Copy link
Collaborator

Thanks 👍🏼 . LGTM. Can you also update the Developer Docs required with this change ?

Nothing needs to be updated. All the make commands remain the same. The only dev aspect updates was on the publishing github actions, the rest should be transparent

Hello @pmbrull - I was referriing to developer docs sections here. Mostly, we use make recipes but if there are any code blocks to be updated, then feel free to get the changes in 👍🏼

@sonarqubecloud SonarQubeCloud
Copy link

[open-metadata-ingestion] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@pmbrull pmbrull merged commit caaf0e7 into open-metadata:main Nov 22, 2023
@pmbrull pmbrull deleted the issue-12436 branch November 22, 2023 06:10
@pmbrull pmbrull mentioned this pull request Nov 22, 2023
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OnkarVO7 OnkarVO7 OnkarVO7 approved these changes

@harshach harshach Awaiting requested review from harshach harshach is a code owner

@akash-jain-10 akash-jain-10 Awaiting requested review from akash-jain-10 akash-jain-10 is a code owner

Assignees
No one assigned
Labels
devops Ingestion safe to test Add this label to run secure Github workflows on PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate setup.py to pyproject.toml
3 participants
@pmbrull @akash-jain-10 @OnkarVO7