-
Notifications
You must be signed in to change notification settings - Fork 60
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
Feat/provenance metadata #334
base: master
Are you sure you want to change the base?
Feat/provenance metadata #334
Conversation
Co-authored-by: Nick Rohn <[email protected]>
We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story. The labels on this github issue will be updated when the story is started. |
- this is for build traceability and provenance Co-authored-by: Nick Rohn <[email protected]>
7576d27
to
53d643d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from a design perspective. I am no Pythonista so I'll leave substantive review to an expert.
"urls": ["[email protected]:org/repo.git"] | ||
} | ||
], | ||
"git_sha": "some-sha", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template above in the metadata.yml file Line 57 has git_sha before git_remotes. Is this out of order here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll check this.
@@ -61,6 +64,12 @@ | |||
# Normalize Jobs - Ensure that job type, template, and properties are set for | |||
# every job | |||
|
|||
def _find_git_root(): | |||
try: | |||
git_root = subprocess.check_output(["git", "rev-parse", "--show-toplevel"]).decode("utf-8").strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always prefer a library call to a subprocess and would recommend using gitpython here: some docs FYI: https://gitpython.readthedocs.io/en/stable/quickstart.html#display-level-1-contents
Also the working_dir may be the same as the git_root expected here, I'm not certain - https://gitpython.readthedocs.io/en/stable/reference.html#git.cmd.Git.working_dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I opted for this is because I had to write a file system walk to locate the root of the git repo when this was originally gitpython. As in, if the tile isn't in the repo root then I'd need to walk up the parent dirs. I'll take a look at both of these.
The standard in other parts of the code for both bosh
and git
is to use subprocess calls:
tile-generator/tile_generator/opsmgr.py
Line 69 in abeca18
def get_credential_dir(update=False): |
def get_credential_dir(update=False):
dir = find_credential_dir()
if update:
devnull = open(os.devnull,"w")
subprocess.call(['git', 'pull'], cwd=dir, stdout=devnull, stderr=devnull)
return dir
I think for these reasons I'm inclined to still use subprocess unless working_dir
pans out (and anywhere else I was adding git
usage). Happy to settle on gitpython if you'd prefer. Third option: a follow up to refactor all the subprocess calls for git so we're consistently either using gitpython or subprocess, but not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A subsequent refactor to maintain consistency would get my vote.
@@ -3,7 +3,8 @@ click>=6.2 | |||
Jinja2>=3.1 | |||
PyYAML>=3.1 | |||
docker-py>=1.6.0 | |||
requests>2.11 | |||
requests<=2.5.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this change related to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was encountering errors while using the latest version of requests: #332
@@ -61,6 +64,12 @@ | |||
# Normalize Jobs - Ensure that job type, template, and properties are set for | |||
# every job | |||
|
|||
def _find_git_root(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended to be a weak internal use function?
https://peps.python.org/pep-0008/#descriptive-naming-styles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments and Questions added. No showstoppers, but things to consider.
If you're inside a repository then the following will be added by default. This can be disabled by passing
tile build --git-info=False
. This is necessary when working outside of a repository.Unit tests pass locally
python -m unittest discover -p '*_unittest.py'
running from the repo root.Ran 146 tests in 2.282s OK
requests
to<=2.5.3
.