Refactoring index CI to a more modular approach. #582
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
#dotests |
mohammedzee1000
force-pushed
the
index_ci_refac
branch
from
605e6e3 to
8137fdd
Compare
|
Sample output so far with test index: |
mohammedzee1000
force-pushed
the
index_ci_refac
branch
2 times, most recently
from
7167c0e to
e797c8e
Compare
|
#dotests |
navidshaikh
requested changes
Jun 20, 2018
ci/container_index/engine.py
Outdated
| from glob import glob | ||
| import ci.container_index.lib.utils as utils | ||
| import ci.container_index.lib.constants as constants | ||
| import imp |
There was a problem hiding this comment.
group the imports like
import a_standard
import b_standard
import a_third_party
import b_third_party
from a_soc import f
from a_soc import g
from b_soc import d
ci/container_index/engine.py
Outdated
| from glob import glob | ||
| import ci.container_index.lib.utils as utils | ||
| import ci.container_index.lib.constants as constants | ||
| import imp |
There was a problem hiding this comment.
where are you using imp? and why ?
There was a problem hiding this comment.
That is an unused import that i added while experimenting. Sorry forgot to remove it.
There was a problem hiding this comment.
@mohammedzee1000 : if you run
pep8 filename.py
flake8 filename.py
pylint --errors-only filename.py
it should help you remove such issues before you even commit to a PR.
ci/container_index/engine.py
Outdated
| import imp | ||
|
|
||
|
|
||
| class Engine(object): |
ci/container_index/engine.py
Outdated
| Initializes the test engine | ||
| """ | ||
| # Index file location needs to be provided | ||
| if not path_exists(index_location): |
There was a problem hiding this comment.
instead of renaming imports, I would import and use
from os import path
[..]
if not path.exists(index_location):
ci/container_index/engine.py
Outdated
| (len(self.index_files) == 1 and | ||
| any("index_template" in s for s | ||
| in self.index_files))): | ||
| raise Exception("No index files to match.") |
There was a problem hiding this comment.
I think the error message should read "No index files to process."
ci/container_index/lib/utils.py
Outdated
| from os import environ, path, mkdir, unsetenv, listdir,\ | ||
| unlink, devnull, getenv, getcwd, chdir, system | ||
| from shutil import rmtree | ||
| from subprocess import check_call, CalledProcessError, STDOUT |
ci/container_index/lib/utils.py
Outdated
| try: | ||
| fnull = open(devnull, "w") | ||
| check_call(cmd, stdout=fnull, stderr=STDOUT) | ||
| return True |
There was a problem hiding this comment.
return True in else block of try
ci/container_index/lib/utils.py
Outdated
| with open(file_path, "r") as f: | ||
| data = yaml.load(f) | ||
| except Exception as e: | ||
| pass |
There was a problem hiding this comment.
so if yaml is not valid a file, we do nothing to report it, refer to my earlier review comment where I pointed out about doing error check while loading user provided yaml files.
There was a problem hiding this comment.
Fixing this
ci/container_index/lib/utils.py
Outdated
|
|
||
| branches_cmd = r"""git branch -r | grep -v '\->' | while | ||
| read remote; do git branch --track "${remote#origin/}" | ||
| $remote" &> /dev/null; done""" |
There was a problem hiding this comment.
add some comments here about what this command is doing and why?
There was a problem hiding this comment.
Comments added.
|
|
||
| system(branches_cmd) | ||
| cmd = ["git", "fetch", "--all"] | ||
| execute_command(cmd) |
There was a problem hiding this comment.
shouldn't the fetch should happen before tracking (above operation)?
There was a problem hiding this comment.
This is how i have always used this command. Still trying to figure out why in this order, but this is what i got from the reference.
Index CI is being re-written to allow modularity and an easier way to use the existing validation.
Depends on validation moved out of this patch.
- Adding error message and flow in case we failed to fetch data from index file. - Adding docstrings. - Fixing imports
- Fixing imports - Moved return True out of exception - load and dump yaml now returns error message that can be used by callee.
- Adding missing docstrings. - Adding default value UNKNOWN to field_name. - Fixing typo.
- Adding docstrings - Changing dicts to dictionaries - Fixing consistency of Id im comments
mohammedzee1000
force-pushed
the
index_ci_refac
branch
from
e797c8e to
5727e05
Compare
mohammedzee1000
force-pushed
the
index_ci_refac
branch
from
f6855ad to
03752e1
Compare
navidshaikh
approved these changes
Jun 22, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the first PR in a series to solve #583.
It aims to achieve the first 2 goals listed in the issue.