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

[AIRFLOW-833] Add an airflow default_config sub-command #2052

Closed
wants to merge 1 commit into from

Conversation

gsakkis
Copy link
Contributor

@gsakkis gsakkis commented Feb 2, 2017

Dear Airflow Maintainers,

Please accept this PR that addresses the following issues:

@jlowin
Copy link
Member

jlowin commented Feb 11, 2017

I suggest that running airflow initdb creates the default config if it doesn't exist already. That way your enhancement allows a more advanced config behavior but the "basic" approach still works.

self.read_string(parameterized_config(TEST_CONFIG))
# then read any "custom" test settings
self.read(TEST_CONFIG_FILE)
def load_or_save_config_file(self, config_file, config_string=None):
Copy link
Member

Choose a reason for hiding this comment

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

This should be split into two methods, load_config_file and save_config_file. Otherwise it will be extremely confusing to figure out what it's doing when reading code.

@gsakkis
Copy link
Contributor Author

gsakkis commented Feb 11, 2017

I suggest that running airflow initdb creates the default config if it doesn't exist already. That way your enhancement allows a more advanced config behavior but the "basic" approach still works.

I'm not sure I understand what you mean here, can you elaborate? Are you suggesting to bundle the proposed functionality into airflow initdb instead of adding a new subcommand?

@jlowin
Copy link
Member

jlowin commented Feb 11, 2017

Nope, I like the new command, I am just suggesting that in addition, the initdb function (https://github.com/apache/incubator-airflow/blob/master/airflow/utils/db.py#L102) should automatically run it if it hasn't been run already

That way the start becomes:

      pip install airflow # required
      airflow default_config # optional, only for advanced users who know they want to customize settings off the bat
      airflow initdb # required, and will generate the default if the previous command wasn't run

@gsakkis
Copy link
Contributor Author

gsakkis commented Feb 11, 2017

Ah ok, makes sense; will do!

@bolkedebruin
Copy link
Contributor

Please also add unit tests that cover the new functions and updated functions.

- Generate the default airflow.cfg explicitly with a command,
  not when simply importing airflow.
- The default airflow.cfg is also created on `airflow initdb`
  if it doesn't exist already.
- Don't create the unittests.cfg unless unit_test_mode is true.
@codecov-io
Copy link

codecov-io commented Feb 12, 2017

Codecov Report

Merging #2052 into master will increase coverage by 0.01%.
The diff coverage is 89.79%.

@@            Coverage Diff             @@
##           master    #2052      +/-   ##
==========================================
+ Coverage    67.7%   67.71%   +0.01%     
==========================================
  Files         139      139              
  Lines       10579    10593      +14     
==========================================
+ Hits         7162     7173      +11     
- Misses       3417     3420       +3
Impacted Files Coverage Δ
airflow/bin/cli.py 53.7% <100%> (+0.51%)
airflow/utils/db.py 84.87% <100%> (+0.39%)
airflow/www/views.py 69.63% <60%> (-0.08%)
airflow/configuration.py 87% <91.17%> (+2.57%)
airflow/task_runner/bash_task_runner.py 93.33% <ø> (-6.67%)
airflow/utils/helpers.py 41.08% <ø> (-2.48%)
airflow/models.py 86.1% <ø> (-0.06%)
airflow/jobs.py 73.12% <ø> (+0.1%)

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 eea5ff8...7de2eff. Read the comment docs.

@gsakkis
Copy link
Contributor Author

gsakkis commented Feb 12, 2017

All comments addressed in the new PR

@ldct
Copy link
Contributor

ldct commented Feb 26, 2017

for consistency with the rest of the commands can we name it like a verb (eg install_default_config) instead

@gsakkis
Copy link
Contributor Author

gsakkis commented Feb 27, 2017

I'm afraid I don't have the time and inclination to jump through any more hoops by an unspecified set of reviewers (presumably core contributors though I can't tell). The way this ticket and especially #2078 have been handled so far don't make me feel that "contributions are welcome and are greatly appreciated" as advertised.

Feel free to make any remaining changes you deem necessary for this PR and merge it, or not.

@stale
Copy link

stale bot commented Dec 11, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 11, 2018
@gsakkis
Copy link
Contributor Author

gsakkis commented Dec 11, 2018

Bump

@stale stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 11, 2018
@ron819
Copy link
Contributor

ron819 commented Jan 3, 2019

@gsakkis note that your code apply changes to the old UI which will be deprecated once #4339 is merged. It's being replaced with RABC.

@gsakkis gsakkis closed this Mar 30, 2019
@gsakkis gsakkis deleted the cli-default_config branch March 30, 2019 09:34
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.

6 participants