Code review comment for ~jocave/certification-docs:add-iot-coverage-guide

Revision history for this message
Rod Smith (rodsmith) wrote :

I concur with Jeff on his point #1. Although the added file builds cleanly to both PDF and HTML when doing a "make all", new Makefile targets to build just this new file are highly desirable. (Without this change, building this one document requires building everything else, and that build could conceivably fail if there's a problem building something else -- say, if a change to rst2pdf causes problems on some documents but not others.) This change could be put into a separate MR, but IMHO it'd be cleaner to just add it to this MR. Given the current Makefile target naming, I'd suggest "iot_coverage" and "iot_coverageh" as target names for PDF and HTML, respectively, but feel free to use what you like.

Changes to packaging rules are a bit iffier. Currently, we build MANIACS and the STG for the Debian package; the other documents are not included in our Debian package. Presumably the IoT documents would not go in our current package, but might go in another one; but this is a matter we might want to discuss to decide on a plan going forward. We currently track changes to the project in debian/changelog, and adding a note about this addition there might be reasonable, although as that file technically tracks changes to the .deb file's contents, if this new document won't go there, omitting changes to this file seems OK. (Note that I've recently submitted another MR [#423024] that makes changes to debian/changelog, so another change to this file will create a conflict unless it's based off, and merged after, my recent MR.) Other packaging file changes would be to debian/control, debian/debinstall, and debian/README.source, if it's decided to add this document to the .deb file.

So to summarize, I'm marking this "Needs Fixing" with the intent that new Makefile entries be added, but packaging changes can wait and will likely either never be made or will create a separate .deb file.

review: Needs Fixing

« Back to merge proposal