-
Notifications
You must be signed in to change notification settings - Fork 166
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
add documentation for validate-downloads job #849
Conversation
@nodejs/build any comments and if not can I get an approval so I can land. |
@mhdawson I added my nit fixes as a |
I'll review properly later today, but as an initial thought I'd rather we didn't start having separate documentation of these jobs, as in my experience it just gets out of date. I'd rather we document the script clearly with comments, and then have the parameters just be the arguments to the script. If the way the parameter names map to the script is clear (it's either an environment variable, or it's passed in directly as an argument to the script) then that should be self-documenting. We could then have a single file that has an overview of each of the jobs to document what we have (i.e. the first paragraph of this doc). I'll try to go through this in the next couple of hours and come up with something concrete. |
@gibfahn waiting on your follow up input. |
So I'd suggest having a single file
|
@gibfahn can land? |
See rationale in comments.
Totally forgot to include this in the last commit.
Okay, pushed a commit to my branch that does what I suggested in earlier comments. LGTM. |
PR-URL: #849 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: JoãReis <reis@janeasystems.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>
Landed as aed300d |
No description provided.