improved uuid checks on commit hook and make targets #3855
Conversation
|
ACK. Missed all that shell magic, but as far as I see two files were just merged into one |
|
@oshtaier regex + grep + sed = magic |
| @@ -3,19 +3,21 @@ | |||
| # This script checks duplicated or empty uuids and exit with 1 if found | |||
|
|
|||
| # finds occurrences of empty @id: testimony tags | |||
| grep -E -i -r -n "@id:(.+[[:blank:]]|$)" tests/foreman/ | |||
| grep -E -i -r -n "@id:(.+[[:blank:]]|$)" tests/foreman/ --include=*.py | |||
| fi | ||
|
|
||
| # Finds occurrences of @id: in testimony tags then | ||
| # sort the output and filters only the duplicated | ||
| # then looks for existence of "@id:" in final output | ||
| # NOTE: can't print the line number -n here because of uniq -d | ||
| grep -r -i "@id:" tests/foreman/ | sort | uniq -d | grep "@id:" | ||
| grep -r -i "@id:" tests/foreman/ --include=*.py | sort | uniq -d | grep "@id:" |
| if [ -n "$EMPTY_IDS" ]; then | ||
| echo "Generating new UUIDS for empty @id tags..." | ||
| else | ||
| echo "No empty @id was found" |
There was a problem hiding this comment.
Do you need to explicitly exit here?
There was a problem hiding this comment.
No need to exit here because I want the other checks in script to work until the end of the script.
| # iterate if any empty @id found | ||
| for output_line in $EMPTY_IDS | ||
| do | ||
| if (echo "$output_line" | grep "tests/foreman"); then |
There was a problem hiding this comment.
Is this check required? Your grep is already pointing at tests/foreman so I expect that all lines would have this?
There was a problem hiding this comment.
I needed to include it here because in some cases grep output have been wrapped, but I can try different approach.
|
|
||
| flake8: | ||
| $(info "Checking style and syntax errors with flake8 linter...") | ||
| @flake8 . --show-source | ||
| @flake8 $(shell git diff --name-only) --show-source |
There was a problem hiding this comment.
This is a good change, but I feel that it is more suitable for travis and commit hook other than checking local changes, I mean I will not be able to run make flake8 for files that I have not added to git yet.
There was a problem hiding this comment.
@elyezer should I change the target name to gitflake or add a new target?
There was a problem hiding this comment.
I mean, that is a personal preference but if others don't worry about it then it is fine to leave instead of creating a separated target.
I've just pointed some of the usecase I personally have.
There was a problem hiding this comment.
changed to gitflake8
| exit 1 | ||
| else | ||
| echo "No @id missing spaces after : was found" | ||
| fi |
There was a problem hiding this comment.
Missing new line at the end of this line.
|
My comments are not a blocker, also don't wait for me to get this merged. |
rochacbruno
force-pushed
the
makefile_improvements
branch
3 times, most recently
from
d82c1ba
to
b1f8baf
Compare
|
Thank you @rochacbruno, will leave to @omaciel have a second look before merging. |
2016-09-27 11:47:39 < omaciel> brunorocha|brb: about https://github.com/SatelliteQE/robottelo/pull/3855 I see that
$ID_TOKEN is defined in 2 scripts. Perhaps it could be defined in one place and
sourced in the other 2 scripts? |
rochacbruno
force-pushed
the
makefile_improvements
branch
from
b1f8baf
to
1230c10
Compare
|
@omaciel fixed! I merged the solution in a single script |
Following recommendations from our meeting.
make uuid-checkandmake uuid-fixmake install-commit-hookincludes a call touuid-fixto fix id issues alsocan-i-push?to check if everyhting is ok@id:xyzmissing whitespace after : as in@id: xyzcheck-uuidagain once PR is sentTODO:
@id(currently we need to keep an empty one to be filled)