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

Fix commands in README.md #242

Merged
merged 1 commit into from
Mar 25, 2022
Merged

Fix commands in README.md #242

merged 1 commit into from
Mar 25, 2022

Conversation

kaxil
Copy link
Collaborator

@kaxil kaxil commented Mar 24, 2022

This PR fixes 4 issues in our README.md:
(1) It changes the date command to make it run on MacOS. Otherwise MacOS users will see the following error:

❯ date --iso-8601=seconds
date: illegal option -- -
usage: date [-jnRu] [-r seconds|file] [-v[+|-]val[ymwdHMS]]
            [-I[date | hours | minutes | seconds]]
            [-f fmt date | [[[mm]dd]HH]MM[[cc]yy][.ss]] [+format]

(2) The dag_id was wrong in airflow dags test command
(3) Fix URL for Airflow homepage
(4) The sqlite_default host is different for MAC vs. Linux so I have fixed that.

@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #242 (908131d) into main (d2d474a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #242   +/-   ##
=======================================
  Coverage   89.88%   89.88%           
=======================================
  Files          67       67           
  Lines        3538     3538           
  Branches      341      341           
=======================================
  Hits         3180     3180           
  Misses        316      316           
  Partials       42       42           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2d474a...908131d. Read the comment docs.

@kaxil kaxil force-pushed the fixes-readme-commands branch 2 times, most recently from b617d76 to 10b1a8a Compare March 24, 2022 22:29
@@ -94,12 +94,25 @@ airflow db init
Create an SQLite database for the example to run with and run the DAG:
```
sqlite3 /tmp/sqlite_default.db "VACUUM;"
airflow dags test calculate_top_animations `date --iso-8601=seconds`
airflow dags test calculate_popular_movies `date -Iseconds`
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be worth updating on line 43 to also have the same name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

L43 is the name of the dag file though, and this is the dag_id

Copy link
Collaborator

@tatiana tatiana Mar 25, 2022

Choose a reason for hiding this comment

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

I know - but it can simplify to the end-user keeping them with the same value in this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed :)

README.md Outdated
```commandline
sqlite3 /tmp/sqlite_default.db "select * from top_animation;" ".exit"
```shell
# The sqlite_default host is different for MAC vs. Linux
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙈 I didn't realise that was the case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn’t know too 😄 - It is because of the following line:

https://github.com/apache/airflow/blob/f06b3955b1d937138fb38021a6a373b94ae8f9e8/airflow/utils/db.py#L560

Which returns a different dir /var/random_nums on my mac🤦‍♂️

@kaxil kaxil closed this Mar 25, 2022
@kaxil kaxil reopened this Mar 25, 2022
README.md Outdated
@@ -94,12 +94,25 @@ airflow db init
Create an SQLite database for the example to run with and run the DAG:
```
sqlite3 /tmp/sqlite_default.db "VACUUM;"
Copy link
Collaborator

@utkarsharma2 utkarsharma2 Mar 25, 2022

Choose a reason for hiding this comment

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

Why do we need this line? isn't it redundant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to use env var to get file name.

This PR fixes 3 issues:
(1) It changes the `date` command to make it run on MacOS. Otherwise MacOS users will see the following error:

```
❯ date --iso-8601=seconds
date: illegal option -- -
usage: date [-jnRu] [-r seconds|file] [-v[+|-]val[ymwdHMS]]
            [-I[date | hours | minutes | seconds]]
            [-f fmt date | [[[mm]dd]HH]MM[[cc]yy][.ss]] [+format]

```

(2) The `dag_id` was wrong in `airflow dags test` command
(3) Fix URL for Airflow homepage
(4) The sqlite_default host is different for MAC vs. Linux so I have fixed that.
@kaxil kaxil requested a review from tatiana March 25, 2022 10:47
@kaxil kaxil merged commit 4f2c75a into main Mar 25, 2022
@kaxil kaxil deleted the fixes-readme-commands branch March 25, 2022 11:25
utkarsharma2 pushed a commit that referenced this pull request Mar 30, 2022
This PR fixes 3 issues:
(1) It changes the `date` command to make it run on MacOS. Otherwise MacOS users will see the following error:

```
❯ date --iso-8601=seconds
date: illegal option -- -
usage: date [-jnRu] [-r seconds|file] [-v[+|-]val[ymwdHMS]]
            [-I[date | hours | minutes | seconds]]
            [-f fmt date | [[[mm]dd]HH]MM[[cc]yy][.ss]] [+format]

```

(2) The `dag_id` was wrong in `airflow dags test` command
(3) Fix URL for Airflow homepage
(4) The sqlite_default host is different for MAC vs. Linux so I have fixed that.
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.

None yet

3 participants