-
Notifications
You must be signed in to change notification settings - Fork 136
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: enable scaffolding for host builds #1750
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lkingland The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
77c545b
to
40dcdc6
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1750 +/- ##
==========================================
+ Coverage 62.04% 63.43% +1.38%
==========================================
Files 100 105 +5
Lines 13311 13405 +94
==========================================
+ Hits 8259 8503 +244
+ Misses 4253 4088 -165
- Partials 799 814 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
40dcdc6
to
0480b8e
Compare
0480b8e
to
1ee740a
Compare
6478ce5
to
feb1d3f
Compare
eede3e7
to
f3c2c2b
Compare
Thanks for the review @lance. The update to the error message you noticed has been made |
74b6912
to
22b077a
Compare
22b077a
to
4695592
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.
Looking really good, @lkingland! I just have a few more comments.
func TestWrite(t *testing.T) { | ||
// The filesystem containing scaffolding is expected to conform to the | ||
// structure: | ||
// /[language]/scaffolding/["instanced"|"static"]-[invocation] |
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.
TODO: we will need to update existing language pack documentation with this kind of information
I have it build locally and tested OK. The code looks like good starting point with tests. I have also tested that existing functionality is still working oK
Looking forward to see more functionality. LGTM |
Co-authored-by: Lance Ball <lball@redhat.com>
Co-authored-by: Lance Ball <lball@redhat.com>
With the exception of refactoring the in-code test helper struct (which I opened an issue for since it might be slightly more involved), all of the comments have been accounted for I believe. Thanks very much for the reviews and verification @lance and @aslom |
/lgtm 🥂 |
/lgtm |
/unhold |
process
Connects the scaffolder to function source code by factoring out scaffolding into a package which is used both by the client and by other packages, allowing for the removal of the mocks.
/kind enhancement