search

siemens / kas

Setup tool for bitbake based projects
MIT License
353 stars 144 forks source link

`KAS_REPO_REF_DIR` bug-fix on kas-container.sh #50

Closed rotem443 closed 3 years ago

rotem443 commented 3 years ago

I hope that I'm not missing anything, but I have tried for a couple of hours to use KAS_REPO_REF_DIR with kas-container without success (while it's worked perfectly using kas without containers).

This little fix solve the issue for me

jan-kiszka commented 3 years ago

Good catch! Seems like no one stressed this so far - since day 1...

Please check https://github.com/siemens/kas/blob/master/CONTRIBUTING.md for the formal process. At least we would need the signed-off to certify DCO.

rotem443 commented 3 years ago

Thanks! sorry for missing out on the sign-off process, let me know if this is ok now

jan-kiszka commented 3 years ago

OK, now bonus for a good patch description:

kas-container: Fix mounting of KAS_REPO_REF_DIR into container

Some words, how this was broken, the seen effect.

Signed-off-by: ...

And also bonus for rebasing your queue to carry only the final commit, not the history (git rebase -i master... / git commit --ammend / git push -f ...). This is not critical here as I would send the commit to the list first (unless you do) and also merge manually. But other projects that do take PRs directly may appreciate such style.

rotem443 commented 3 years ago

Hope I managed to do that now, Thanks for the details! I would take it into consideration on the next PR

jan-kiszka commented 3 years ago

Almost: There are still 2 commits in your branch (https://github.com/rotem443/kas/commits/master). If you do git rebase -i HEAD~2, you can mark the second commit as 'squash' and then edit the combined commit log when being asked to so that it carries both the proper subject as first line and the commit message with signed-off. Force-push that single commit, and I can pick it up.

rotem443 commented 3 years ago

Thanks for all the details! appreciate that. It seems good now

jan-kiszka commented 3 years ago

Thanks, sent to the mailing list and merged into next.