►
From YouTube: Code Review Weekly Workshop - Sep 09, 2022
Description
In this session we talk about reviewing dependency upgrades, changes to GitLab's reviewer and maintainer roles, and we pair up on reviewing an indeterminate MR.
A
B
Yeah,
first
of
all,
yeah
thank
you
for
organizing
this
call.
I
find
it
like
super
interesting.
I
just
learned
about
it
when
you
posted
it
in
the
slack
channel
and
yeah.
Just
in
time
like
I
had
an
interesting
review
yesterday
and
it
was
a
dependency
update,
and
I
also
saw
that
you
discussed
some
dependency
updates
on
this
call
before
and
like
they're
always
a
challenge.
Yes,
so
do.
B
Okay,
here
I'm
opening
the
mr,
so
it's
a
first.
This
was
actually
upgraded
to
1.5
or,
I
think,
or
some
of
the
latest
version
right
and
so
first
thing
I
look
at
when
reviewing
dependency.
Updates,
of
course,
is
a
change
log
right.
It's
like
the
obvious
thing
to
check,
but
you
know
it's
always
good
to
us
to
also
check
the
code,
because
you
know
some
things
might
not
be
in
the
change
log
and
in
this
case
that
change
that
bug
wasn't
obvious
from
this
change
log.
B
My
diff
and
something
with
the
versions
you
could
change
the
versions
if
you
want
to
div
one
older
version
to
this
new
version.
Oh
cool
and
what's
nice
about
this-
is
that
this
actually
diffs?
You
know
the
actual
contents
of
the
gem
right,
because
sometimes
we're
tempted
to
just
go
to
the
sidekick
crown
repo,
and
then
you
know,
look
at
the
the
tags
here
and
do
a
compare,
but
that's
for
security
reasons.
That's
not
the
best
thing
to
do,
because
you
know
the
what's.
B
The
contents
of
the
repo
is
not
exactly
I
mean
could
be
like
you
know,
poisoned
or
whatever,
and
the
gem
contains
some
other
code.
So
it's
a
good
idea
to
do
things
like
this.
I
don't
know
if
yarn
packet,
npm
packages
have
something
similar,
but
this
is
for
ruby,
gems.
We
have
this
thing
and
so
yeah
and
for
this
one
it's
kind
of
I
wouldn't
say
I
we
should
like
spend
so
much
time
into
reviewing
these
gems.
B
It's
always
daunting,
because
you
know
you
don't
have,
like
you,
don't
know
the
whole
code
base
of
that
gem
right
and
but
for
this
one
I
was
like
contributing
to
this
sidekick
polar
code,
and
so
I
know
this
code
base,
so
I
kind
of
like
familiar
with
it.
So
it's.
This
is
not
like
expected
that
you
find
bugs
in
upstream
stuff,
but
I
was
just
lucky
to
be
familiar
with
the
code
here
and
so
yeah.
We
found
a
bug
and
then.
C
B
Polling
is
supposed
to
depend
on
a
the
number
of
processes
running
so
that
they
get
random.
You
know,
if
you
have
five
processors,
then
every
process
should
only
ping
like
longer
periods
so
that
you
don't
get
so
many
things.
Yeah
and
the
sidekick
maintainer
is
very
responsive.
I
found
the
patch
it
got
merged
same
day,
so
let's
go
wow.
C
A
That
is
a
really
cool
thing
to
share,
and
it's
kind
of
really
scary
for
everyone
to
see,
because
these
dependency
update
mr's
are
a
little
scary,
because
a
whole
lot
is
changing.
It's
a
whole
lot.
You
don't
have
control
over
and
you
kind
of
you
know
like
to
see
examples
of
where
I
didn't
need
to
spend
all
that
time.
But
this
is
an
example.
If
you
didn't
need
to
spend
all
that
time,
and
that's
it's
tough,
that's
really
tough.
A
Until
yeah,
I
think
the
big
question
is
like
I'd
love,
to
hear
your
thoughts
on
this
heinrich
and
anyone
else
like.
What's
what
is
the?
How
do
you
distinguish
if
this
is?
If
this
is
a
dependency
update
that
warrants
that
level
investigation
or
not,
and
because
it
is
a
lo?
It
is
a
lot
and
it's
a
lot
that
you
know
some
people
may
not
be
familiar
with.
So
what
are
your
thoughts
on
making
that
decision
heinrich.
B
You
know
it's
like
kick
round
breaks
all
that
we
do
a
lot
of
crown
jobs
and
the
brakes
or
you
know
if
it
breaks
sidekick.
In
this
case,
the
bug
is
actually
funny,
because
what
it
does
is
it's
altering
sidekick
behavior,
instead
of
just
you
know
altering
its
own
behavior,
the
crown
behavior.
So
you
know
it.
B
A
A
C
D
D
A
A
I
think,
if
we're
changing
something
that
like
oh,
it
would
interact
change
how
we'd
interact
with
that
database
layer
under
different,
and
this
database
layer
is
now
assuming
just
one
protocol
like
that.
Would
that
would
cause
issues
and
then
the
more
you
move
that
up
the
layered
architecture.
I
think
that's
when
that
causes
issues,
but
it
is
worth
looking
into
and
it's
worth
just
pinging
someone
if
you're
not
sure
about
it.
If
it's
something
as
critical
as
like
sidekick,
that
is
talking
to
the
database,
you
know
if
and
that's
an
issue.
D
A
End
a
and
back
end,
b
and
back
in
a
is
using,
updated
sidekick
which
uses
a
new
you
know
of
some
sort
of
breaking
change
data.
It's
going
to
write
to
the
back
end
then
yeah,
whatever
worker
b,
that
doesn't
have
that
update,
won't
be
able
to
read
that.
So
that's
a
good
point
too
yeah.
That's
a
really
tough
requirement
to
meet
because
on.com
like
we're
pretty
good.
If,
like
we
do
these
canary.
A
The
our
on
I
realized
last
time
I
looked
at
that:
it's
not
just
dot-com
reliability,
but
apparently
we
have
a.
Maybe
I
shouldn't
say.
Apparently
we
do
have
a
commitment
to
customers
that
if
they
they
can
subscribe
to
this
zero
downtime
upgrade
stream
if
they
just
upgrade
from
one
version
to
another
version,
and
that
adds
that's
a
that's
hard
to.
It
just
adds
a
bit
of
unintuitive
complexity
to
how
we
do
changes
but
yeah
thanks
for
bringing
that
up
terry.
A
D
D
But
yeah,
I
definitely
don't
go
into
the
level
like
I've,
only
gotten
a
few
dependency
updates
to
review,
and
even
when
it's
something
that
I'm
really
familiar
with
like
we,
I
we
just
did
the
elastic
search
gym
not
too
recently
and
it's
scary.
So
I'm
with
you
on
that
one,
it
seems
so
simple.
It's
just
like,
like
other
often
like
a
few
lines
of
changes,
yeah.
A
B
A
D
A
A
D
D
F
A
You
don't
want
this
on
the
recording
well
too
bad,
recording,
please
wow,
yannick.
What
you
just
said
was
really
profound
and
probably
changed
my
life
forever.
Thanks
for
bringing
that
up
absolutely
on
talking
about
dependency
upgrades,
I
do
really
want
to
check
and
with
natalya,
because
you
just
did
view
big
view
upgrade
and
did
we
happen
to
see
any
any
quirkiness
of
like
maybe
was
there
any
issues
where
we
discovered
some
quirky
behavior,
with
the
new
view
that
we
had
to
push
upstream
fixes
or
something
like
that
or
what
was
did.
E
I
think
you
know
the
answer
I
just
wanted
to
be
recorded,
so
yeah
there
was
an
aftermath
that
was
really
impossible
to
check
on
their
review,
and
this
aftermath
is
growth
of
compile
assets
size
and
it's
not
just
a
little
growth.
It
was
really
significant
one
that
goes
to
our
pipelines
running
for
much
longer,
taking
much
more
resources.
E
So
we
are
averted
the
change
in
the
end-
and
this
is
something
that
is
really
hard
to
find
on
the
review
stage
or
even
on
testing
a
new
package,
because
you
can't
see
it
on
the
gdk
and
even
when
you
check
it
on
the
mr,
you
have
a
slight
increase
in
term
bundle
size,
but
not
the
way
it
affects
the
compiled
assets
so
yeah.
We
need
to
revert
this
one
and
still
thinking
how
the
result
that
the
loader
problem.
A
Let
that
be
bubbled
up
the
warning
of
like
hey
some,
mr,
is
about
to
really
change
the
com
and
we're
trying
to
do
that
with
the
bundle
stuff.
But
I
know
it's
that's
touch
and
go
is
super
reliable,
but
those
are
tough.
The
kind
of
situations
that
you
really
don't
get
to
know
about
until
it's
you
know
we're
considering
reverting
now
and
that's
yeah.
Okay,
thanks
for
sharing
italia,.
B
Yeah,
I
just
remembered
something
else:
that's
similar,
like
I
remember
doing
a
ruby
gem
upgrade
before
where
it
has
c
extensions
and
what
happens
is
like
we
package,
our
gitlab.
You
know
omnibus,
in
different
platforms
and
turns
out
it
doesn't
compile
on
certain
architectures
and
all
that
and
you
find
out
later
too
late,
right
and
yeah.
A
B
Good
thing
is:
I
saw
a
slack
message
about
distribution,
adding
a
new
job
to
our
pipeline,
where
you
know
it
does
a
package
on
all
platforms
or
something
I
forgot,
the
name,
but
that's
a
good
thing
to
do.
When
you
see
a
dependency
update
that
you
know
touches
like
c
extensions
or
whatever
local
thing
that
could
affect
other
platforms.
A
Yeah,
that's
a
good
point
cool.
Well,
I
I
hope
I
don't
have
a
dependency
upgrade,
mr
anytime
soon,
I'm
joking!
No,
I
actually
feel
more
equipped
to
handle
them
and
the
way
that
serves
our
customers
best.
So,
thanks
for
the
discussion,
did
you
want
to
bring
up
some
of
the
things
you're
observing
with
the
maintainer
saffionic?
Or
do
you
want
to
hop
to
acid.
F
F
So
I'd
be
curious
about
your
take
on
this.
I
don't
specifically
have
a
well.
I
do
have
a
lot
of
opinions
on
this,
but
I'm
I'm
taking
this
as
granted.
There
were
not
not
many
surprises
to
me
starting
off
with,
though
I
feel
like.
I
have
found
at
least
one
contradiction
in
there,
a
quick
one
to
start
with,
and
that
would
be
our
reviewer
values
and
we
could
probably
discuss.
A
F
F
Joking,
but
it's
actually
code
related
and
very
much
so
in
this
case,
because
what
I
want
to
highlight
is
something
that
I
would
still
highlight
here
here.
We
are
lean
towards
a
bias
for
action,
and
collaboration
for
velocity
don't
be
afraid
to
to
fix
linting
areas.
Like
summarize,
if
you
find
a
really
small
thing,
just
fix
it
and
go
ahead
because
it
might
be
yeah
just
a
typo
or
something,
let's
not
block
anybody
just
do
it,
but-
and
I'm
like
you-
can
help
me
out,
because
you've
been
very
active
in
this
threat.
A
Do
you
mind,
do
you
mind
if
I
hop
in
so
this
leaning
towards
the
bias
for
action?
Collaboration
is
has
been
in
the
handbook
for
a
year
plus
just
recently,
for
some
reason,
maybe
pushing
like
we
need
to.
We
need
to
show,
without
a
shadow
of
a
doubt,
to
some
customers
the
kind
of
controls
that
we
have
internally.
A
That's
there's
some
sort
of
external
reason.
That's
wanting
us
to
be
able
to
quickly
show
without
a
shadow
of
doubt,
there
are
no
unauthorized
changes
hitting
our
repo
and
there's
a
lot
of
interpretation
of
what
unauthorized
change
means,
but
one
way
would
be
hey.
Every
change
goes
through
review
and
I
think
that's
some
sort
of
way
that
that
gets
interpreted
and
so
yeah.
This
is
a
this
is
not
the
intent.
It's
not
the
not
the
intent
is
sorry.
A
B
B
Like
not
updated
because
I
remember
there
was
an
mr
because
we
had
this
like
documented
in
the
review,
something
page
in
the
handbook-
and
I
remember
the
mr
to
change
the
approval
rules,
also
change
that
to
remove
the
that
section
where
you
can
apply
your
own
suggestion
thing.
Maybe
it
was
a
different
handbook
page
and
this
one
wasn't
updated
so
yeah.
A
I
think
that's
the
case.
This
is
a
different
handbook,
page,
so
yeah
unfortunately,
I
mean
I
would
say,
here's
the
interesting
thing,
though,
is
it's
really
just
maintainers
can't
do
that
now,
and
hopefully
we
push
hard
to
figure
out.
You
know
hey,
this
is
an
efficiency.
Hud
doesn't
go
with
our
values.
We
need
to
figure
out
a
way
to
not
only
support
inquiries
of
unauthorized
changes,
but
also
allow
maintainers
to
do
this.
A
A
F
D
Can
someone
help
merge
this
for
me
and
then
I
guess
I
guess
my
only
worry
about
that
is
like
as
a
maintainer,
it's
hard
for
me
to
contact
switch
into
like
an
mr
and
just
say:
oh
like
I
don't
want
to
just
go
and
say:
okay,
I'll,
just
go
approve
it
without
like
looking
at
it
and
then
like.
How
much
do
you
look
at
it?
D
C
A
Not
sure
what
to
write,
that's
a
great
point,
but
I
think
there's
a
there's
a
hidden
cost
with
what
did
it?
What
would
they
call
it?
Taking
the
big,
the
thick
paintbrush
and
trying
to
do
this
big,
broad
fix,
which
you
know,
hurts
this
practice
that
we've
done
is
it's
gonna
in
the
end,
hurt
the
health
of
our
code
base
because
now
there's
friction
towards
bringing
things
up
and
fixing
things
that
are
just
right
there
glaringly
glaringly
obvious
of
like
hey.
We
can
just
fix
this
and
merge
it
and
go
now.
A
There's
friction
and
I
think
people
will
be
less
motivated
to
promote.
You
know
that
level
of
health
in
the
code
base
because
it's
gonna,
you
know,
add
a
couple
of
days
now
before
the
mr
gets
merged
and
something
that
we
need
to
be.
I
I
believe
that
we're
wanting
to
address,
but
I
guess
for
some
reason
we
had
to
we
had
to
meet
some
sort
of
requirements
first,
so
these
are
definitely
I
I
think
we
could
do
better
at
being
transparent
on
the
reasons
why
some
of
those
changes
were
made.
A
I
don't
know
if
it's
in
this-
I
don't
know
if
it's
in
this
thing
too.
So
here's
what's
interesting,
no
you're
right,
it's
not
in
this
one.
So
this
is
what's
so
weird,
it's
like
so
yeah.
We
have
this
huge
maintainer
working
group
putting
together,
you
know,
making
consistent
getting
rid
of
contradictions
and
we're
going
to
try
to
plot
this
course
for
what
does
being
a
reviewer
and
our
maintainer
mean,
but
then,
at
the
same
time
we
did
this
huge
culture
shift
of
using
code
owners.
C
D
I
was
in
the
maintainership
working
group,
I
didn't
realize
the
other
change
was
happening,
but
I
also
haven't
gone
to
a
ton
of
the
working
group
meetings
just
because
the
time
was
just
not
convenient
for
me,
but
I
have
been
working
on
some
of
the
changes.
I
think
there's
still
like
one
outstanding,
mr
with
regards
to
the
trainee
program
that
has
yet
to
be
emerged
but
yeah,
I
definitely
kind
of
threw
some
interesting
changes
into
like
what
we
have
been
discussing.
D
C
A
C
A
A
I
know
it's
weird
pairing
pair
reviewing
is
weird,
but
I
like
to
have
this
dedicated
time
for
me
to
get
on.
I
I
don't
do
any
reviews
during
the
week
anymore.
I
just
do
it
during
this
meeting,
I'm
joking,
but
I
always
have
a
sundry
of
reviews
that
we
can
take
a
look
at,
but
if
you
all
have
interest
interesting,
whether
they're
we
don't
even
have
to
finish
them,
but
just
sometimes
opening
them
up
and
talking
about
them
can
be
helpful.
A
A
Okay,
all
right.
I've
shared
this
screen.
This
one
is
like
already
in
the
third
round
of
review
and
is
fairly
small
and
from
the
front-end
perspective,
and
this
is
probably
going
to
be
this-
the
final
round
of
review
so
that
might
be
cool.
This
is
the
dependency
update.
I've
been
procrastinating
on.
A
I
know
I
know
I'm
bad.
This
is
my
fault,
I'm
sorry
and
I'm
sorry
source
crap.
Fine.
I
apologize
to
source
craft
too.
This
is
one
on
the
web
id
project
which
could
be
interesting.
This
is
one
that
is
new
to
me.
It
is
on
the
gitlab
ui
project.
So
I
haven't
read
this
at
all
and
this
might
be
fairly
straightforward.
A
A
F
F
B
A
F
A
B
A
A
A
A
So
it's
this
is
disabled
and
is
there
in
the
indeterminate
one
and
then
terminal
1
is
not
being
hit
here,
but
it
does
look
like
I
I'm
not
going
to
try
to
change
it
here,
so
I
do
want
to
see
it
make
sure
we
fix
it
with
this
thing,
so
that
is
actually.
I
would
like
to
confirm
this
ui
css
actually
fixes
this,
and
I
can
do
that
in
the
gitlab
ui
project.
We
have
a
really
cool
job.
A
I
can
run
which
will
create
a
integration
branch,
and
then
I
could
just
open
up
the
review
app.
You
know
sure
we
dropped
twenty
dollars
running
the
pipeline,
but
spending
good
lab
money
like
it's
my
own,
I'm
joking,
which
means
on
credit.
Joking
too,
I'm
sorry
so
this
will
create
an
mr
actual.
This
will
create.
A
A
And
so
here
in
the
story,
I
would
have
expected
us
to
have
recreated
this
in
the
story
that
original
issue,
but
that's
not
necessarily
what
I'm
saying
that's
okay,
so
the
big
to-do
for
me
is
I'd
like
to
confirm
that
this
fixed
the
issue
in
a
transparent
way.
I'm
going
to
actually
just
leave
a
note
for
myself
about
that.
C
C
A
A
A
C
A
A
E
A
Interesting
interesting
so
we're
seeing
if
this
story
is
like
determinant,
it
should
change
the
rest
of
the
story.
This
is
all
none
of
this
is
front.
None
of
this
is
user
facing
stuff.
This
is
all
just
our
stories,
but
can
I
ask
you
another
question?
I
feel
I'm
sorry
terry
and
heinrich
you're
here
I'm
going
to
go
into
the
nitty-gritty
of
the
front
end
stuff,
but
I
thought
dot.
Sync
did
something
like
you're
describing,
but
I
was
unfamiliar
with
it
and
we
also
use
v
model.
So
now
we
we
just
do.
E
A
E
C
A
But
it's
clearly
causing
me
some
level
of
confusion.
You
know
it
might
cause
others
levels
of
confusion.
This
is
worth
me
time,
boxing
myself,
so
I'm
going
to
time
box
myself
for
five
minutes
by
myself
I
mean
the
collective
us
which
are
doing
this
together.
I'm
going
to
run
storybook
and
I'm
gonna
see
does
anything
actually
change
here
like
it's,
and
if
I'm
not
seeing
how
this
changes
or
what
this
is
doing,
then
I'm
gonna
ask
a
question
about
it.