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

KibbleConfigParser #74

Merged
merged 7 commits into from
Oct 27, 2020
Merged

KibbleConfigParser #74

merged 7 commits into from
Oct 27, 2020

Conversation

skekre98
Copy link
Contributor

@skekre98 skekre98 commented Oct 24, 2020

I've made KibbleConfigParser a subclass of configparser.ConfigParser. Currently the class is assuming that the ini file has been configured correctly. Let me know if you'd like me to do some error handling if this is not the case.

The usage of this class is as follows:

from configuration import KibbleConfigParser

# Reads kibble.ini by default
conf = KibbleConfigParser()
conf.read("kibble.ini")

for section in conf:
    for key in conf[section]:
            print(conf[section][key])

# Can also be used as follows
# conf.get(section, key)

Output

True
True
2
0.1.0
kibble
elasticsearch
9200
False
localhost
25
Kibble <noreply@kibble.kibble>

I have also created a class variable called uri which is in the format dbname://host:port. Let me know if this is the format you were looking for.

@skekre98
Copy link
Contributor Author

Apologize for the dirty commit log, took me a second to realize pre-commit was being used 😅

kibble/setup/kibble.ini Outdated Show resolved Hide resolved
kibble/setup/kibble.ini Outdated Show resolved Hide resolved
kibble/setup/parser.py Outdated Show resolved Hide resolved
@turbaszek turbaszek linked an issue Oct 24, 2020 that may be closed by this pull request
kibble/setup/parser.py Outdated Show resolved Hide resolved
@skekre98
Copy link
Contributor Author

skekre98 commented Oct 25, 2020

Not sure why CI error is persisting, working tree is clean even after pre-commit run --all-files. Any suggestions on how to solve?

@turbaszek
Copy link
Member

Not sure why CI error is persisting, working tree is clean even after pre-commit run --all-files. Any suggestions on how to solve?

Have you done git add after running pre-commit?

kibble/setup/parser.py Outdated Show resolved Hide resolved
kibble/setup/parser.py Outdated Show resolved Hide resolved
kibble/setup/parser.py Outdated Show resolved Hide resolved
@skekre98
Copy link
Contributor Author

skekre98 commented Oct 25, 2020

Not sure why CI error is persisting, working tree is clean even after pre-commit run --all-files. Any suggestions on how to solve?

Have you done git add after running pre-commit?

Yes, I run the commands as follows:

$ pre-commit run --all-files
$ git add .
$ git commit -m "msg"
$ git push origin master

This is the output of the pre-commit command:

(home) Sharvils-MacBook-Pro:kibble sharvilkekre$ pre-commit run --all-files
Check Yaml...............................................................Passed
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
Fix python encoding pragma...............................................Passed
Add license for all other files..........................................Passed
Add license for all rst files............................................Passed
Add license for all md and html files................(no files to check)Skipped

Am I missing something?

@turbaszek
Copy link
Member

@skekre98 please rebase, you are missing last changes from #66

git remote add apache git@github.com:apache/kibble.git
git fetch --all
git rebase apache/master
git push --force

@skekre98
Copy link
Contributor Author

I apologize for the delay. Still getting used to the repo workflow 😅. Let me know if there is anything I left out.

@turbaszek
Copy link
Member

@skekre98 would you mind refactoring kibble/setup/setup.py so it uses this config for default values?

@skekre98
Copy link
Contributor Author

@skekre98 would you mind refactoring kibble/setup/setup.py so it uses this config for default values?

Could you elaborate on this a bit? Are you looking for me to refactor get_parser and return an instance of KibbleConfigParser?

@turbaszek
Copy link
Member

@skekre98 would you mind refactoring kibble/setup/setup.py so it uses this config for default values?

Could you elaborate on this a bit? Are you looking for me to refactor get_parser and return an instance of KibbleConfigParser?

Once we have configuration file for Kibble we should use it in the application:

  1. create instance of Kibble config config = KibbleConfigParser()
  2. use it as config.get("section", "key") for default values in https://github.com/apache/kibble/blob/d7f9031dfd93a2efd676fcbd59443feec01df6ed/kibble/setup/setup.py#L43-L54

This will also required some refactor of the setup script + docker-compose to be sure it works.

Let me know if you would like to try do this or should we merge this PR as it is.

@turbaszek turbaszek changed the base branch from master to main October 27, 2020 16:01
@skekre98
Copy link
Contributor Author

Should we perhaps merge this, and address setup.py in a different issue? I feel like this may require a bit more understanding from my end which we could discuss on the other issue. I will likely have some elaboration questions when I begin working on it as well ☺️.

@turbaszek turbaszek merged commit 2abfcc8 into apache:main Oct 27, 2020
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.

Refactor the configuration methods of Apache Kibble
2 participants