-
Notifications
You must be signed in to change notification settings - Fork 10
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
PSCE-302 feat: adds a task to sync third party content to a local trestle workspace #137
Conversation
…ot is valid AuthoredObjects need to exists in valid trestle workspace. Handling the case that the working directory is not initialized with trestle. Signed-off-by: Jennifer Power <[email protected]>
Reduces code duplication in the end to end tests and any test that has git and trestle checks Signed-off-by: Jennifer Power <[email protected]>
The adds one source type, git-managed, and copies models in the supported trestle model list (e.g. catalogs, profiles) from a git repository to a local trestle workspace. Signed-off-by: Jennifer Power <[email protected]>
Signed-off-by: Jennifer Power <[email protected]>
@JimFuller-RedHat PTAL |
validator, | ||
) | ||
logger.info(f"Successfully copied from {source}") | ||
finally: |
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.
There might be a good reason not to handle any specific exceptions here ... though maybe a log.info here could give a hint if this function is ever called elsewhere.
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 didn't consider the potential for this to be called elsewhere. I think you are right. It would probably be better to handle the exceptions here and make execute()
a bit simpler.
) | ||
self.sources = git_sources | ||
self.validate = validate | ||
super().__init__(working_dir, model_filter) |
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.
curious if model_filter has any applicability in execute()
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.
Good question. Since all tasks are used for workspace maintenance operations, the model_filter
is used in a concrete method on the TaskBase
class that allows certain models to be skipped based on criteria (at the moment it just based on filepaths). In this task, it is called in copy_validate_models
to allow certain paths to be skipped in the the clone repo during the copying process.
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.
apart from minor questions, LGTM
It makes sense to move the exception handling closer to where the exceptions are being thrown and allows this method called in other places. Signed-off-by: Jennifer Power <[email protected]>
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.
LGTM, reviewed trestlebot/tasks/sync_upstreams_task.py in more detail during paired code review session today
Description
A
SyncUpstreams
Task to sync third party content/artifact dependencies in a trestle workspace to a local trestle workspace.Review Hints
Recommended to review each commit separately.
The first two commits have preparatory changes to reduce technical debt/code duplication that would be proliferated by this feature.
Rationale
We need automation that will keep our third party content update to date that works across CI providers. We have a bash script currently performing this in one location.
This is a basic implementation for a "trestle aware" copy from upstream repository.
For future iterations, the goal is to start simple and built a task that can extend to different types of sources (i.e. release artifacts) and more selective copy options (e.g. using resolvers in
trestle
for dependency resolution).Type of change
How has this been tested?
Checklist