Support additional details with status command #1781
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
1 similar comment
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
* Added `-out` flag to `dep status` command.
* The new flag accepts a single argument. Users can mention
the file path where the status output should be written to.
* Blank values are ignored.
* When a file path is specified, the status output will be
written to a file at the specified file path. If the file
cannot be created (for some reason), a meaningful error
message will be printed on the stdout.
Tests:
- dep status -?
Shows the information about the new -out flag
- dep status -out junk/file/path
A meaningful error is printed
- dep status -out proper/file/path
The status output is printed to the file at the
mentioned file path.
* Added the dependency on `build` target for `test` target. * Added a simple test case to verify the output of `dep status -out` command.
akutz
force-pushed
the
feature/status-details
branch
from
5386411 to
adf851e
Compare
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
1 similar comment
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
Two commits 'a71121a' and 'edbfaa3' are pulled from my tree. I provide my consent to include both the commits in this request. |
I also consent to having my commits included. |
This patch introduces a default Go template for emitting the status details in a format that is a duplicate of the lock file. There are several tests that validate the default template. The tests can be extended to validate additional templates as well.
Enhance dep status to support detailed output for the tabular, JSON, and text/template output formats. * For each of these formats, the detailed output includes source repository information and a list of packages used by each project (instead of simply the count). * For the JSON and text/template output formats, this detailed view separates branch, version, and revision information (for both locked and latest) to simplify programmatic processing of the output. * For the JSON format, solve metadata is included in the output. Additionally, this sets the stage for an all-at-once text/template outputter (like the JSON outputter, instead of the current line-by-line approach preserved from the basic text/template outputter) to allow a template to additionally process metadata information.
Instead of processing the detailed template output line-by-line, like the basic template, evaluate it all-at-once.
Document the use of the detail flag by itself and with others.
This patch updates the status.go source file using the go fmt command.
This patch renames the `defaultStatusTemplate` to `statusLockTemplate` in order to avoid confusion.
This patch updates the imports list in status.go so that the list is ordered properly.
This patch updates two lines to return errors directly as recommended by the CI tool, code climate.
akutz
force-pushed
the
feature/status-details
branch
from
85d23fa to
3198c75
Compare
|
Hi, thanks for putting so much of work in this. This feature is useful and we need this. We had a In the command spec, we have defined a mode of operation called project argument, which gathers a lot of more information about projects than what we do while fetching BasicStatus. It was implemented in PR #1367 and you can see #1367 (comment) for what the obtained details look like. It contains all of the details that's added in this PR and a lot more. With that, I would imagine providing all those details to template would be great. But I'm afraid that would take some time. In the meantime, if it's about fixing #1773, I think we can add It could be that I'm overlooking something that you guys are trying to add here. Please let me know. I would love to accept all the work you have here, but we might want to achieve the same result in a different way. @sdboyer please correct me if the above plan is missing something. |
|
Hi @darkowlzz, Thank you so much for taking the time to review this! So the issue with the branch name and version as is is that it’s necessary to parse it out instead of being discreet. For a project that opted to have separate version, revision, and branch constraint fields, I think it should also be specific in the template contract. Please note this PR also simplified the way the data is emitted, with all the callbacks following the cumulative approach a la the JSON writer. It just makes more sense when emitting a document model. Personally I think the greatest value of this PR are the structs that represent the document model for the template, JSON, etc. They divorce the internal status from the model used to emit the status. That’s beneficial for long term stability IMO. Regardless, if this PR is not accepted as is then I feel the following must be true of what replaces it:
Thank you again for the review! If Sam didn’t tell you, our team won a global OSS hackathon with this PR :) |
|
(Please pardon above typos; I’m on my cell) |
|
Hi @darkowlzz, I just reviewed your PR. My response would be that it makes sense to take your work and implement it as a new flag on top of this PR like this PR’s -lock flag. The latter just uses a built in Go template, so why not create a new flag called -vhead or -2cols that uses a Go template to emit the details in your PR’s vertical header / two-column layout? |
|
Hi @darkowlzz, Please allow me to say that I really appreciate your name being Sunny and your username being dark...owlzz :) |
| The -f flag allows you to use Go templates along with it's various | ||
| constructs for formating the output data. Available flags are as follows: | ||
| The -f flag allows you to use Go templates along with it's various | ||
| constructs for formating output data. Available flags are as follows: |
|
|
||
| When used with -detail, the -f flag applies the supplied Go templates | ||
| to the full output document, instead of to packages one at a time. | ||
| Available flags are as follows: ` + availableDefaultTemplateVariables + ` |
There was a problem hiding this comment.
Newline here before the availableDefaultTemplateVariables
|
OK, finally had a minute to start looking this over. The big, immediate question: what are we gaining with i do agree, certainly, that we want a contract on the output variables that are available to the template, and documenting those is excellent. i haven't absorbed the PR enough yet, though, to see where it does or doesn't cross over with #1367. |
I think the main value of It also allows us to preserve symmetry between the various formats: tabular, JSON, template. The same information is included for each of those formats, for both "normal" and "detailed" output. I think there's some value here: no one is going to switch from one format to another only to find that there is a key piece of information that is not included in the other format. A change of a format affecting the information that is included may be unintuitive/surprising for users. However, there is already some precedent for different formats having different information: dot (which includes less information than the other formats). If we accept that different formats can include different information, it's easier to remove
If we don't care about backward compatibility for users currently using |
|
To add to what Zach said, part of our intention was not to change the existing code or results too much, but rather be additive. |
There was a problem hiding this comment.
Thanks for your patience, folks - so much of my time has been consumed with rebutting vgo that it's been difficult to get to any open PRs! 😭
You make good points about the backwards compatibility issues. i'm convinced - all of it can be more holistically revisited in the successor tool.
What does this do / why do we need it?
This PR fixes #1773 by enhancing
dep statuswith the-detailflag and ability to emit status details equal or greater than the information found in the lock file.The command
dep status -lockis equivalent todep status -detail -f 'TEMPLATE'whereTEMPLATEis a Go template that formats the detail data as a perfect duplicate of the dep lock file. The data contract emitted to the template is:Thus anyone can use
dep status -detail -f TEMPLATEwith a custom Go template and the above object model to produce any custom version of the lock data required. For example, the following:is the same as:
What should your reviewer look out for in this PR?
Planes, trains, and automobiles. The reviewer should execute
dep status -detailwith all of the various formatter options, such as:dep status -detaildep status -detail -jsondep status -detail -lockdep status -detail -f TEMPLATEThe reviewer should also ensure that the existing architecture of the status command was augmented, not replaced.
Do you need help or clarification on anything?
Are there additional formats that should be added as flags to
dep status, such asdep status -glide, etc.?Which issue(s) does this PR fix?
fixes #1773