-
Notifications
You must be signed in to change notification settings - Fork 63
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
Dockerfile Outerloop Support #580
Conversation
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 haven't done a very precise review but caught some stuff. I'll do another round after the left ones get responses
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.
As we briefly discussed, I think having all fields at the same level in the spec could cause confusion (with the main example being the gitLocation
field at the level as id
and uri
. I've opened PR maysunfaisal#1 as an example of what it might look like to shuffle around fields for clarity.
Suggested update to PR devfile#580
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 apart from a couple minor things and the still-to-do tasks from Serhii's 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.
Just a few minor suggestions but apart from that nothing to add.
LGTM
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Moves GitLocation in DockerfileImage Image component type under the Git struct to make clear that this field is intended only for use when specifying a git repo. (Files not regenerated for readability) Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
@sleshchenko could you pls re-review, I've updated the PR with your feedback |
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. One question that can be addressed separately (perhaps a topic for the weekly call?)
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: amisevsk, maysunfaisal, sleshchenko The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Maysun J Faisal maysunaneek@gmail.com
What does this PR do?:
Which issue(s) this PR fixes:
Fixes #51
PR acceptance criteria:
Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened.
Unit/Functional tests
QE Integration test
Documentation
Client Impact
How to test changes / Special notes to the reviewer:
I've provided some devfile samples on how a devfile with outerloop build & deploy will look. There are also many snippets of the new component for tests that can be used as a guide or reference