Enforce job name length to 255 characters in the database

marked this issue as related to #342800 (closed)

added backend candidate14.8 devopsverify grouppipeline authoring sectionops severity2 typebug + 1 deleted label

@lauraX - this is the follow-up issue we discussed to provide more confidence in this change prior to %15.0. I placed it as a candidate14.8 given end of the year and it's well in advance of the next major release. WDYT?

cc: @dhershkovitch - this is a follow-up issue to ensure more confidence/near-zero risk by nudging up that size a little more.

Thanks @marknuzzo! I think 14.11 might be the last 14.x milestone, but if we are releasing it in 15.0, should it have the 15.0 label?

Thanks @lauraX for pointing that out. I had been misinterpreting that label as the intended milestone for it to be worked on but I can tentatively mark this issue's target milestone for 14.8.

@marknuzzo no, it was definitely me who messed that up, sorry! I was thinking about it the wrong way when I read this. You are 100% correct

Thanks @lauraX for the follow-up. No problem and I'm glad I didn't misinterpret. I have adjusted it back to candidate14.8

changed milestone to %Backlog

added [deprecated] Accepting merge requests label

added priority2 label

added candidate15.0 label and removed candidate14.8 label

changed milestone to %14.8

added candidate14.8 label and removed candidate15.0 label

@marknuzzo and @dhershkovitch,

This grouppipeline authoring bug has at most 25% of the SLO duration remaining and is ~"approaching-SLO" breach. Please consider taking action before this becomes a ~"missed-SLO" in 14 days (2022-01-10).

added SLONear Miss label

changed due date to January 14, 2022

assigned to @dhershkovitch

removed [deprecated] Accepting merge requests label

@marknuzzo Do you believe we can de-prioritize this issue since we've implemented #342800 (closed) (deprioritize = reduce the ~severity/priority and put this into our backlog)

Hi @dhershkovitch - thanks for the ping here. I think that since we've extended this to 255 characters already, I think it would be ok to deprioritize and reduce priority (your call) and severity for now. @lauraX - please weigh in with your thoughts too. Thank you!

@marknuzzo - I think this is fine. Are we still planning on enforcing database validation in %15.0 though? With 255 or 500 characters, whatever is appropriate, it should be the same thing.

Hi @lauraX

Are we still planning on enforcing database validation in %15.0 though?

Yes, I would feel more comfortable if we still adjusted the character limit from 255 -> 500 as a part of %15.0. Depending on how much lead time is needed here (i.e. weight), we can determine when this can be worked either next month, March or April since 15.0 rolls out in May. WDYT?

/cc @dhershkovitch

@marknuzzo

I would feel more comfortable if we still adjusted the character limit from 255 -> 500 as a part of %15.0.

Is there an issue to track this?

@marknuzzo sounds good! I think this should be an easy backend-weight1 , since it only requires a database validation. We can do it all the way in April.

@dhershkovitch this could be the issue for it maybe?

Is there an issue to track this? @dhershkovitch this could be the issue for it maybe?

Hi @dhershkovitch - yes, this is the issue that we would track the character limit adjustment for %15.0 from 255 -> 500. So we don't lose track of this issue, do you want to change the milestone to 15.0 or add the candidate15.0?

/cc @lauraX

Added the weight and label!

Yes we can, can we make sure the issue description is up to date @marknuzzo @lauraX ?

@dhershkovitch updated

cc @marknuzzo

Thanks to both of you!

@marknuzzo - As I was doing some cleanup, I realize we can probably do this issue as part of this feature flag rollout cleanup: #344665 (closed).

Also since it's not a breaking change, we can probably just do it sooner rather than later, since the MR is ready anyway. WDYT? If you agree, we can close this issue and I will submit the MR.

Thanks @lauraX - I agree that this appears to be the same thing as that MR that is already ready. We can close this issue since the issue's MR is already moving. Closing issue now...

added severity4 label and removed severity2 label

changed milestone to %Backlog

removed candidate14.8 label

added needs weight label

added priority4 label and removed priority2 label

removed due date

unassigned @dhershkovitch

removed needs weight label

set weight to 1

added backend-weight1 label

added [deprecated] Accepting merge requests label

changed milestone to %15.0

changed title from Extend job name length to 500 characters to Enforce job name length to 500 characters in the database

changed the description

mentioned in issue gitlab-org/ci-cd/pipeline-authoring#44 (closed)

mentioned in issue gitlab-com/www-gitlab-com#12906 (closed)

Hello, I'd like to work on this issue in case nobody else is working on it, are you still interested in contributions?

mentioned in issue #344665 (closed)

@marknuzzo @lauraX is this issue considered to be breaking change (will it change the default behavior) or is this more of a tech debt?

@dhershkovitch - this was considered to be more tech debt than a breaking change, but I guess it would be a breaking change if someone has a job name that exceeds 500 characters, like so:

Hello, I am a job name with more than five hundred characters. Perhaps I am just very descriptive? Maybe. I am just long, I'm still typing and haven't even reached two hundred. Today I ate some bread, cheese, and eggs for breakfast. I will probably have some pizza for dinner, but I'm not sure yet. I have been craving a Philly cheesesteak so maybe that will be better, although caramelized onions take a really long time to cook and I am not feeling up for it. Back to the job, we're almost there:

This is VERY unlikely. And we've been enforcing 255 via AR model validation anyway with no issues. 500 is def overkill

@lauraX

should we update the issue title to be 255 characters instead of 500?

@dhershkovitch I'm fine either way! It is mentioned in the description that it might be safer for us to do 500 just in case, although 250 is STILL pretty long. @marknuzzo wdyt - should we keep 255 or go for 500?

@dhershkovitch @lauraX Given other higher priorities at the moment, I'm fine with staying with 255 characters. That would mean we can close this issue as this was only to increase to 500 to completely cover us which technically wouldn't break anything but allow for more characters in the job name. As Laura noted though, I think we're safe. Closing for now.

@marknuzzo - sorry, I should've been more clear, we still need to do the database validation! Right now it's only on the backend (via ActiveRecord) rather than the database. However - this is an easy weight 1, the MR shouldn't take long, we can probably do this between meetings.

@dhershkovitch

Ah thanks @lauraX for clarifying my misunderstanding here. That works for me. Thank you

added breaking change label

closed

reopened

@lauraX @marknuzzo If we're proposing to enforce the 500 char limit via DB constraints and we currently return a 500 error for any input of >255 characters, where is the breaking change? (Is it an API call that surpasses some FE form input?)

@cheryl.li - That is a good point. When we first talked about this issue, it was considered a breaking change because we didn't enforce the limit, and at least I wasn't aware of the 500 being returned. So to your point - this is not a breaking change anymore :)

cc @marknuzzo

@lauraX Thank you, I removed breaking change

Thanks @lauraX for the additional explanation here around the original intent of breaking change.

Some of the original concern was around by enforcing 500 characters, that would put us in a much safer place but is it possible that a user could have created a job name with 500+ characters, which we then labeled as breaking change. After further consideration and analysis, I think everyone arrived at that having a job name at 500+ characters may be near impossible so the label can be removed, which I have done now.

/cc @dhershkovitch

@marknuzzo - do you think we should just make it 255, given that the page encounters a 500 after 255 anyway?

Hi @lauraX - yes I agree that we should just align it with 255 for enforcing that way the error isn't encountered in the future. I'll update the description now.