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

#14028: fix the sed command issue in ingestion dep script #14029

Merged
merged 12 commits into from
Dec 3, 2023
Merged

#14028: fix the sed command issue in ingestion dep script #14029

merged 12 commits into from
Dec 3, 2023

Conversation

allenkallz
Copy link
Contributor

@allenkallz allenkallz commented Nov 19, 2023 •
edited by pmbrull

Describe your changes:

Fix #14028

  • Test issue with one hash
    fix_password_test_with_one_hash

  • Test issue with two combine hash

fix_password_test_with_two_combine_hash

  • Test issue with two different hash

fix_password_test_with_two_different_hash

I worked on ... because ...
I encountered this issue during the setup of the Airflow ingestion container. To address this, I employed Terraform to generate a randomly generated password, resulting in an RDS password that includes the special character '#'

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.

@allenkallz allenkallz requested a review from a team as a code owner November 19, 2023 22:42
@github-actions GitHub Actions
Copy link

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@allenkallz allenkallz changed the title fix the sed command issue in ingestion dep script Fixes 14028: fix the sed command issue in ingestion dep script Nov 19, 2023
@github-actions GitHub Actions
Copy link

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@github-actions GitHub Actions
Copy link

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@github-actions GitHub Actions
Copy link

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@github-actions GitHub Actions
Copy link

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@harshach
Copy link
Collaborator

@allenkallz Thanks for your contribution. We will review it and merge it in

@harshach harshach changed the title Fixes 14028: fix the sed command issue in ingestion dep script Fixes #14028: fix the sed command issue in ingestion dep script Nov 22, 2023
@harshach harshach changed the title Fixes #14028: fix the sed command issue in ingestion dep script Fix #14028: fix the sed command issue in ingestion dep script Nov 22, 2023
@harshach harshach added the safe to test Add this label to run secure Github workflows on PRs label Nov 22, 2023
@harshach
Copy link
Collaborator

@allenkallz as per @pmbrull
AIRFLOW__DATABASE__SQL_ALCHEMY_CONNenv var. Will take precedence over airflow.cfg
https://airflow.apache.org/docs/apache-airflow/stable/howto/set-config.html

So can we get rid of this sed and use the above parameter. We can document it as part of this PR.
Let me know if that works for your usecase

@pmbrull pmbrull changed the title Fix #14028: fix the sed command issue in ingestion dep script #14028: fix the sed command issue in ingestion dep script Nov 23, 2023
@allenkallz
Copy link
Contributor Author

@harshach Yes this AIRFLOW__DATABASE__SQL_ALCHEMY_CONN env variable also works for me. will update the PR .

@dhruvinmaniar123 dhruvinmaniar123 self-assigned this Nov 23, 2023
dhruvinmaniar123
dhruvinmaniar123 previously approved these changes Nov 23, 2023
View reviewed changes
Copy link
Contributor

@dhruvinmaniar123 dhruvinmaniar123 left a comment

Choose a reason for hiding this comment

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

Thanks @allenkallz. This looks good.

@harshach
Copy link
Collaborator

@allenkallz can you update the PR to use the AIRFLOW__DATABASE__SQL_ALCHEMY_CONN variable please. Will merge it in

@allenkallz
Copy link
Contributor Author

@allenkallz can you update the PR to use the AIRFLOW__DATABASE__SQL_ALCHEMY_CONN variable please. Will merge it in

Hi @harshach I update the PR , Thanks

@github-actions github-actions bot removed the devops label Dec 3, 2023
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Dec 3, 2023

[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

@harshach
Copy link
Collaborator

harshach commented Dec 3, 2023

Thanks @allenkallz for the PR. Merging it in.

@harshach harshach merged commit b50f6f8 into open-metadata:main Dec 3, 2023
12 checks passed
MrVinegar pushed a commit to MrVinegar/OpenMetadata that referenced this pull request Dec 15, 2023
@allenkallz
open-metadata#14028: fix the sed command issue in ingestion dep script (
432d66b 
open-metadata#14029)

* fix the sed command issue in ingestion dep script

* remove sql_alchemy_conn from airflow.cfg and use env variable

* try execute ingestion dep script without source
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmbrull pmbrull pmbrull left review comments

@harshach harshach harshach approved these changes

@dhruvinmaniar123 dhruvinmaniar123 dhruvinmaniar123 left review comments

@akash-jain-10 akash-jain-10 Awaiting requested review from akash-jain-10

Assignees

@tutte tutte

@akash-jain-10 akash-jain-10

@dhruvinmaniar123 dhruvinmaniar123

Labels
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.

Airflow ingestion dependency script sed command error
6 participants
@allenkallz @harshach @dhruvinmaniar123 @pmbrull @tutte @akash-jain-10