Fork me on GitHub
#babashka-sci-dev
<
2021-12-10
>
cap10morgan19:12:39

@rahul080327 @borkdude let me know if my comments on the PR don't make sense. I've had to learn a lot about multi-platform docker builds lately and it is a lot 🙂

lispyclouds19:12:06

Its looking really nice @cap10morgan I'm learning about it too! knew a bit about manifest but mostly hid behind the buildx 😅 awesome stuff!

borkdude19:12:45

I'm just glad you guys are figuring this stuff out :-D

cap10morgan19:12:51

🙂 yeah I hadn't done much w/ manifest before either. I'm ripping it out of my PR now though haha

cap10morgan21:12:31

here goes nothin...

cap10morgan21:12:34

oh except it won't do anything interesting b/c of the branch filters. I'll let if run on this branch too for now.

cap10morgan21:12:02

Oh but it's still a snapshot so no joy

borkdude22:12:40

should I move the version back to 0.7.0 - does that help?

cap10morgan22:12:16

yeah that would let it trigger, but I think I need to do it on my branch. so probably easier if I just do it. BUT I'm nervous about letting it clobber those images on Docker Hub...

borkdude22:12:12

the docker token isn't exposed to pull requests anyway :)

cap10morgan22:12:19

ok good! 🙂

cap10morgan22:12:25

alright giving that a shot

cap10morgan22:12:16

hmm, got a test failure that seems unrelated / intermittent, but I don't have perms to re-run. I guess I can push another dummy commit though if you don't mind the clutter 🙂

borkdude22:12:31

oh yes, those hato tests use httpbin and that's a flaky service

👍 1
cap10morgan22:12:47

hahaha, oh boy. the check in the .circleci/script/docker script is if [ -z "$CIRCLE_PULL_REQUEST" ] && [ "$CIRCLE_BRANCH" = "master" ] so it still didn't run

cap10morgan23:12:15

given that .circleci/config.yml already has branch filters and docker hub tokens aren't shared with pull requests, what do you think about just removing that?

cap10morgan23:12:53

I'm going to do it in this PR to test, but let me know if you'd like me to put it back before merging

borkdude23:12:51

why would we remove them? I don't want to push images in non-master branches?

borkdude23:12:12

the tokens aren't shared with PRs from outside the organization

borkdude23:12:49

Please make the PR as minimal as possible, not introducing any unnecessary changes

borkdude23:12:07

That's a good practice in general

cap10morgan23:12:36

yes, I agree of course. but I don't want to merge something that's untested!

cap10morgan23:12:43

these things are guarded against in other ways

cap10morgan23:12:52

but like I said, I'm happy to put it back before merging

borkdude23:12:35

yeah, I got it. for testing it's ok.

cap10morgan23:12:37

for example, the docker job won't even run in non-master branches due to the circleci config

cap10morgan23:12:56

hence why I had to whitelist my branch

borkdude23:12:03

maybe make a note in the PR as a TODO that we add it back before merging

cap10morgan23:12:19

luckily the version change made it non-mergeable so you can't even do it accidentally now 😅

cap10morgan23:12:47

woof, more unrelated test failures

borkdude23:12:27

perhaps uncomment the tests for now - saves on circleci credits

borkdude23:12:39

I mean comment

borkdude23:12:04

also comment out the jvm stuff, etc, everything you don't need to for testing and to get faster feedback

borkdude23:12:30

I'll check back tomorrow. Thanks for looking into it!

👍 1
cap10morgan23:12:56

sounds good I'll do all that if I get another failure

borkdude23:12:55

I was thinking we could have a skip-testsfilter to skip tests in branches if you're not interested in those

borkdude23:12:10

for intermediate commits

borkdude23:12:17

since they can take quite long

cap10morgan23:12:53

that's a good idea

cap10morgan23:12:07

haha and this time it got as far as not having docker hub creds which is pretty early in the process. I should probably go back to having it push to my docker hub account and add that to the pre-merge checklist

cap10morgan23:12:16

think I'm wrapping up for the day too, though