Skip to content

Add template for compare xml job#3

Open
akaRem wants to merge 1 commit into
ci-team:masterfrom
akaRem:initial-stuff-1
Open

Add template for compare xml job#3
akaRem wants to merge 1 commit into
ci-team:masterfrom
akaRem:initial-stuff-1

Conversation

@akaRem
Copy link
Copy Markdown
Collaborator

@akaRem akaRem commented May 4, 2017

This patch adds template and couple of useful macro
for performing xml comparisements of JJ changes

@akaRem akaRem force-pushed the initial-stuff-1 branch from 6ebe80a to 3cbcd2b Compare May 4, 2017 20:12
@akaRem akaRem requested review from bookwar and ibelikov May 4, 2017 20:25
@akaRem akaRem self-assigned this May 4, 2017
Copy link
Copy Markdown

@bookwar bookwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking on doing that comparison a bit differently.

Instead working in the same workdir with two different commits, we just checkout the same repo twice into two different subfolders.

destinationBranch to dest/ and PR to pr/ and then run tox two times in exh of the subfolders.

The trade-off is that we need to checkout the same repo twice, but then it will simplify the bash logic a lot and we get much cleaner logic in the end.

@akaRem akaRem force-pushed the initial-stuff-1 branch from 3cbcd2b to 29b07d7 Compare May 5, 2017 16:02
@akaRem
Copy link
Copy Markdown
Collaborator Author

akaRem commented May 5, 2017

Well, I just ported stuff we had before.
We could rework this job later.. but this job works for us.

@akaRem akaRem force-pushed the initial-stuff-1 branch 5 times, most recently from 1166eb1 to d9d7e58 Compare May 6, 2017 14:38
This patch adds template and couple of useful macro
for performing xml comparisements of JJ changes
virtualenv venv
source venv/bin/activate
pip install pip --upgrade
pip install tox
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO: this is node configuration and it is not responsibility of this script.

mv "./output/${ENV##*/}" "./output/jobs/new/${ENV##*/}"
done

compare_xml() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the function declaration break flow of main logic, and it is hard to read this script.
It would be better to move this function in top of file or just remove it, because there is only one place where this function is called.

@@ -0,0 +1,67 @@
- job-template:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO: this job is related to stash and its name or location should reflect this fact.

host: '{host}'
username: '{username}'
password: '{password}'
credentials-id: '{credentials-id}' No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no newline at end of file.

password: ''

jobs:
- library/compare-xml No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no newline at end of file.

ADDED="[added]<br>"
REMOVED="[removed]<br>"

DIFF=$(diff -q -r -u "${OUT_DIR}/old" "${OUT_DIR}/new" &>"${LOGFILE}"; echo "${?}")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The layout of output folder is out of scope of the compare+xml function. Thus instead of ${OUT_DIR}, ${OUT_DIR}/old and ${OUT_DIR}/old we should have FROM_DIR and $TO_DIR or A_DIR and B_DIR as diff usually uses this naming.

I would also move comparison function to a separate utils/compare.sh script. And call it in the JJB step as ./utils/compare.sh output/new output/old > report.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants