You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metron.apache.org by mattf-horton <gi...@git.apache.org> on 2017/02/15 08:47:01 UTC
[GitHub] incubator-metron pull request #455: METRON-720 modify generate-md.sh to re-t...
GitHub user mattf-horton opened a pull request:
https://github.com/apache/incubator-metron/pull/455
METRON-720 modify generate-md.sh to re-throw errors from within 'find'
This patch also fixes, en passant:
METRON-719 use of quadruple back-ticks in README.md file
which is the problem that caused the need for METRON-720.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mattf-horton/incubator-metron 0.3.1-rc4
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-metron/pull/455.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #455
----
commit 34d420718edac283342c3e3cbdacab4e07846ddd
Author: mattf-horton <mf...@hortonworks.com>
Date: 2017-02-15T08:36:37Z
METRON-720 modify generate-md.sh to re-throw errors from within 'find', and
METRON-719 use of quadruple back-ticks in README.md file
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by JonZeolla <gi...@git.apache.org>.
Github user JonZeolla commented on the issue:
https://github.com/apache/incubator-metron/pull/455
Spun it up on CentOS 6.8 and macOX 10.12.3, and did a quick code review. Spot tested the traps as well. +1 (non-binding).
The only thing I noticed is I don't think [this](https://github.com/mattf-horton/incubator-metron/blob/0.3.1-rc4/site-book/bin/generate-md.sh#L300) is necessary due to the `>` in the following line.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by mattf-horton <gi...@git.apache.org>.
Github user mattf-horton commented on the issue:
https://github.com/apache/incubator-metron/pull/455
@JonZeolla , sorry traveling yesterday. Will look at it this evening.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by mattf-horton <gi...@git.apache.org>.
Github user mattf-horton commented on the issue:
https://github.com/apache/incubator-metron/pull/455
Hi @JonZeolla , thanks for taking a look amidst the other demands on your time.
I disagree with 'mv $fullpath/site.xml.bak $fullpath/site.xml' for the following reasons:
* site.xml is now a fully auto-generated file (like the contents of site/markdown/), and is no longer tracked in git. That was the METRON-717 improvement. Thus, there is no reason to restore it.
* There are many other artifacts of bin/generate-md.sh besides site.xml, that cannot be restored to former state, and that are likely to be just as broken as site.xml if the build is interrupted. I don't want to give the impression that we've restored a valid overall context.
* Leaving the entire state of the auto-generated content after an interrupted build, including site.xml, may give valuable information about the root cause of the break.
Please accept my not incorporating this change :-)
BTW, for Jon and all reviewers, I should mention that after this patch I did a full re-validation of the site-book, both by:
* visual inspection in a browser of every page of the resulting book, and by
* recursive diff of both the generated MD source files and resulting HTML book files, before and after this whole PR patch.
No unexpected differences were found. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/incubator-metron/pull/455
Hey @ottobackwards site.xml is now in .gitignore with @mattf-horton's latest changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by JonZeolla <gi...@git.apache.org>.
Github user JonZeolla commented on the issue:
https://github.com/apache/incubator-metron/pull/455
PR to your branch is open. Feel free to merge --squash without attribution, or not merge and go forward without it. +1 (non-binding) the current state by inspection.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/incubator-metron/pull/455
re: "site.xml is now a fully auto-generated file" that would be why it is showing in git as unstaged but modified?
Is there a way we can stop that from happening? If it is generated does it need to be in git?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by mattf-horton <gi...@git.apache.org>.
Github user mattf-horton commented on the issue:
https://github.com/apache/incubator-metron/pull/455
I've tested it to completion on both Centos 6 and 7. It works and generates identical src and site file trees. I also tested the trap on SIGINT, which worked on all three platforms. I tested the trap on ERR but only on Mac.
@JonZeolla , the 'rm -f outfile' before 'blah 2> outfile' may be unnecessary. I did so on the premise that the file might pre-exist and be readonly. In that case, I'm not sure whether the '>' operator deletes the file, or attempts to rewrite it; the former would succeed, the latter would fail. Whereas the 'rm -f' is guaranteed to succeed, because we earlier tested the parent directory for write privs.
At any rate, as you observed, it is at worst unnecessary but harmless. Thanks for the +1.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/incubator-metron/pull/455
Tested branch locally, with and without 4 backticks. Looks good to me. +1
Side note, I noticed that the sensors link doesn't go anywhere.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by mattf-horton <gi...@git.apache.org>.
Github user mattf-horton commented on the issue:
https://github.com/apache/incubator-metron/pull/455
I have pushed fixes for all of the above. To fix item 3 I had to change history, so you'll need to "pull -f" to update an existing image.
I tested code fragments and confirmed that "basename -s" works fine on Centos7 but breaks as described by @JonZeolla on Centos6. The revised code fragments work on both and on Mac OS X.
I'm now testing the whole site-book implementation on both Centos 6 and 7. Will report results.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/455
Ok, yep, I'm +1 on this. Also, I agree with the reasoning around not having a bak file. Especially compelling is the first bullet point that `site.xml` is generated now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by JonZeolla <gi...@git.apache.org>.
Github user JonZeolla commented on the issue:
https://github.com/apache/incubator-metron/pull/455
Sounds good to me. I typically use declare for all variables just to be explicit and obvious with my intent, and use $() as a standard for readability and easy nesting as opposed to \`\`. That said, as you mentioned, they are definitely equivalent to what you have across the board.
I did a quick review on my phone, but there are enough changes it probably merits a run up and validation on a screen larger than 5". That said, I would consider adding something like:
```
If [ -e $fullpath/site.xml.bak ]; then
mv $fullpath/site.xml.bak $fullpath/site.xml
fi
```
To the quit function to reinstate the prior site.xml (replacing `$fullpath` appropriately). I'll try to spin this up today if I can because I'm gone all next week without access to the real world (i.e. Internet).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by mattf-horton <gi...@git.apache.org>.
Github user mattf-horton commented on the issue:
https://github.com/apache/incubator-metron/pull/455
@JonZeolla , thanks for the suggestions. Incorporated the SIG traps and better exit codes, as well as improvements to the 'find -exec' error catching logic. Also added 'set -e' and trap on ERR, and fixed an exit code in the fix-md-dialect.py script.
Changes related to $() vs ``, and explicit declaration of array variables that are being initialized on first use, are valid alternative syntax, but my usages are equivalent. I consider the variants I used to be less cluttered, so I kept them.
While I was at it, I also fixed METRON-717, splitting site.xml into a tracked template file and an untracked auto-generated portion, in git.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/455
Either as @JonZeolla says or you could also do `basename $md .md`. I do think running on centos is important as eventually I'd like to get this running as part of travis, so I'm willing to hold my +1 until the basename issue is corrected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by JonZeolla <gi...@git.apache.org>.
Github user JonZeolla commented on the issue:
https://github.com/apache/incubator-metron/pull/455
Nice catch. When I get to my laptop I'll throw you a PR with some of my standard bash script features, since it's getting modified anyway. Just a couple of minor things.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/incubator-metron/pull/455
ok - i'm not building this branch, just what i've seen from other builds
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron pull request #455: METRON-720 modify generate-md.sh to re-t...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-metron/pull/455
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/incubator-metron/pull/455
Hm, that's only when building with the bin scripts. re sensor links
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by mattf-horton <gi...@git.apache.org>.
Github user mattf-horton commented on the issue:
https://github.com/apache/incubator-metron/pull/455
I just checked, and this PR branch rebases to master without conflict, which I think means it will merge without conflict. If anyone would consider committing it, let me know if you need me to rebase it first. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/455
@ottobackwards yeah, we probably need site.xml to be in the `.gitignore`, right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by JonZeolla <gi...@git.apache.org>.
Github user JonZeolla commented on the issue:
https://github.com/apache/incubator-metron/pull/455
I'm not going to have time to review this more than I already have. +1 (non-binding) via inspection. If the PR is still open in 1 1/2 weeks I can take another stab at a review, but don't wait on me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by mattf-horton <gi...@git.apache.org>.
Github user mattf-horton commented on the issue:
https://github.com/apache/incubator-metron/pull/455
Hi @JonZeolla , @ottobackwards , @cestella , @mmiklavc ,
Thanks to all of you for looking at this. Taking issues in order:
1. Absolutely right, no reason to have site.xml.bak any more! Missed that, will fix momentarily.
2. I'll have to look at that basename issue, but will fix and test on Centos. Glad you noticed, and sorry about that.
3. The weird behavior with site.xml and .gitignore is because the "git rm site.xml" command and the change to add "site.xml" to .gitignore, were squashed into a single commit. If you do it as a sequence of two commits then it transpires cleanly. But I was trying to respect the preference for squashing commits. It won't manifest in fresh "git clone"s, but is a nuisance for folks pulling into an existing branch.
Since I have to make another commit to fix #1 and #2, I will also split #3 back into separate commits. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-metron issue #455: METRON-720 modify generate-md.sh to re-throw er...
Posted by JonZeolla <gi...@git.apache.org>.
Github user JonZeolla commented on the issue:
https://github.com/apache/incubator-metron/pull/455
I'm a -1 (non-binding) on this for now, primarily because the bash fails to run on CentOS 6.8 (where I tested it). The error was `basename: invalid option -- 's'`.
I think I able to fix the issue by changing that line to `item_name=$(basename ${md%.*})` (feel free to sub backticks, just hard to show in GitHub md)
Regarding the prior discussions
I guess I fail to see the reason why site.xml.bak is created in the first place then. A reasonable alternative in my mind would be to add something like `diff $fullpath/site.xml $fullpath/site.xml.bak && rm $fullpath/site.xml.bak` to `quit()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---