-
Notifications
You must be signed in to change notification settings - Fork 0
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
Create artefact store #8
Conversation
Taking this back, it actually fails some CI tests that I must have missed :( |
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.
Hi @hiker , I have reviewed the changes related to this pull request. They all look good to me with only one typo found. As the tests have all passed, I have approved this.
Thanks for the review. I've updated the PR, flake8 and CI seems to be fine. Ready for next review, |
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. Now I think it is ready to merge.
The artefact store is originally just a dict. This introduces a new object that as the artefact store (which inherits from dict, so it behaves the same), but it allows us to add additional methods if required.
It also adds a getter in the BuildConfig (to avoid accessing the private member
self._artefact_store
)). I also fixed a few coding style issues (again something we need to figure out :) )