Replace weight icon

added Community contribution UI polish frontend labels

added 1 commit

cd3397b2 - Replace weight icon

marked the checklist item Changelog entry added, if necessary as completed

@pedroms some tests will probably fail but this should be ready to for UX review, could you take a look? /cc @akaemmerle because https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/7847#note_107482041.

added Hackathon label

changed milestone to %11.6

Great, thanks for this, @gtsiolis! I was very close to getting this in myself.

assigned to @pedroms

approved this merge request

@gtsiolis awesome, LGTM!

@kushalpandya can you review the FE here?

assigned to @kushalpandya

Thanks @gtsiolis, only 1 suggestion regarding CSS styling, rest looks good.

assigned to @gtsiolis

added 176 commits

cd3397b2...d786cf32 - 175 commits from branch gitlab-org:master
0b4f2dc2 - Replace weight icon

Compare with previous version

Thanks @kushalpandya, could you take another look when you're free? Also, congrats on becoming a maintainer!

assigned to @kushalpandya

Hold on! Let me take a look at the failing test first!

@gtsiolis Since app/assets/stylesheets/pages/issuable.scss is part of Core, we'd need a CE MR with the changes backported for ee-specific-lines-check to pass.

assigned to @gtsiolis

@kushalpandya yup, I'm on it. Also, tests will be easily fixed now that we're using a class in the icon, too!

added 1 commit

fa90a16e - Replace weight icon

Compare with previous version

@kushalpandya I've ported the style changes in https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/8448. I wonder why this line (min-width: 0;) is missing from CE?

Likely that the change was never backported to CE before Let's not backport that change as we may have to test if that causes a regression or not.

approved this merge request

resolved all discussions

@kushalpandya shouldn't we merge this before merging the CE port? This was mentioned in the CE upstream (see https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/8488).

assigned to @kushalpandya

mentioned in merge request !8488 (merged)

When an EE MR involves CE backport, it is always wise to merge CE first, and if we're not close to freeze and the backport involves significant changes, it is better to wait further until CE to EE for that day happens, and then EE MR can be rebased against latest EE master (which also includes changes from CE backport trickled down to EE), and this ensures we don't run into major conflicts in CE to EE MR. This isn't a formally written process, but I personally insist on it as it is more efficient.

In this case, CE MR was minor and also, we were not close to freeze so merging CE MR first still makes sense (see that this MR no longer has any failing jobs).

merged

mentioned in commit 3b08e954

@kushalpandya I don't think your comment is 100% accurate. We always recommend merging the EE version first (https://docs.gitlab.com/ce/development/ee_features.html#backporting-changes-from-ee-to-ce):

Once EE MR is merged, the MR towards CE can be merged. But not before.

The reason for this is precisely the problem that we see with the EE MR.

There is an exception if the EE MR is going to be open for much longer than the CE one, and the CE one is 100% self-contained in both CE and EE. If they are both for the same feature and both ready, we should generally merge the EE one first.

cc @godfat @rymai to correct me if I missed anything

@smcgivern Weird, as I always noticed that when I have CE backports be merged first, especially if it involves too many changes (and the MR itself has ee_compat_check passing), CE to EE conflicts won't be a problem once EE MR is rebased against latest EE master and then merge.

Having said that, in this particular MR, there was no concrete reason to merge CE first, just that the CE MR finished pipelines early and I had MWPS set on it, while EE MR was still failing on ee-specific-lines-check. My comment mentions general practice that I follow in given conditions. Of course if that makes CE to EE MR problematic, I'd surely change the order of merge.

and the MR itself has ee_compat_check passing

I think this part depends on why that check is passing. It can pass simply because there's a matching branch in EE - so if that branch isn't merged, we will have problems. If it's passing because there isn't a matching branch, and it's checking master, then there are no issues.

@kushalpandya Please always merge the EE merge request first. The exceptions are rare enough that I think we could discuss on a case by case basis, and never a general rule. Reading your comment at https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/8448#note_118147160 surprised me a lot that why you understood that way. That was never the process as far as I know.

My understanding is completely the same with @smcgivern, and I would also like to emphasise that:

Passing ee_compat_check doesn't mean we could merge CE first, because as Sean pointed out:

It can pass simply because there's a matching branch in EE - so if that branch isn't merged, we will have problems. If it's passing because there isn't a matching branch, and it's checking master, then there are no issues.

ee_compat_check will check against the EE branch first, meaning that if it passed, it doesn't mean it's fine for EE master. If there's no EE branch, no EE MR, then yes it's fine to merge CE MR, because the job will be checking against EE master. However if we're thinking about having an EE MR, I think we're certainly having a matching EE branch. In that case we should merge EE MR first.

We're speeding up the process of CE upstream merge requests train. Merging CE branch first will be more and more likely to have issues in the future, because it'll be more likely that the CE branch immediately goes to the CE upstream merge request, and we could potentially resolve the same conflicts twice, or abandon the EE MR in the end. Eventually we don't have an issue, but the effort put onto EE MR was wasted.

Thanks for clarifying on our stand about the process @smcgivern @godfat! What I mentioned was purely based on my experience in the past where EE MR when merged before CE would run into conflicts when CE to EE merge happens (after CE MR is merged). And that didn't happen if backported MR was merged before EE.

I agree on ee_compat_check part as I've also had false positives previously where the Job would pass but CE to EE merge won't happen clean. However, I'll keep this in mind while merging MRs to ensure that EE gets merged first, even if it is running into ee-specific-lines-check failure and we're aware which files it is complaining about. Thanks again!

@kushalpandya No problems

where EE MR when merged before CE would run into conflicts when CE to EE merge happens (after CE MR is merged). And that didn't happen if backported MR was merged before EE.

To my experience, this can happen around the prepend line which is expected to be EE specific therefore we can't really avoid the conflicts. To my understanding, there's no way we could really avoid that, and the conflicts resolution is usually trivial enough because it's just one or two lines, unless we have a lot of EE specific lines around that area. In that case... we should work toward reducing the difference.

Thanks for firing up the discussion @smcgivern @godfat @kushalpandya! While I'm no expert in backporting changes back to CE, I was really curious why CE was merged before EE, especially after the relevant conflict in CE upstream (see https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/8488).

Passing ee_compat_check doesn't mean we could merge CE first, because as Sean pointed out:

It can pass simply because there's a matching branch in EE - so if that branch isn't merged, we will have problems. If it's passing because there isn't a matching branch, and it's checking master, then there are no issues. ee_compat_check will check against the EE branch first, meaning that if it passed, it doesn't mean it's fine for EE master. If there's no EE branch, no EE MR, then yes it's fine to merge CE MR, because the job will be checking against EE master. However if we're thinking about having an EE MR, I think we're certainly having a matching EE branch. In that case we should merge EE MR first.

See a related proposal to make it clearer: https://gitlab.com/gitlab-org/gitlab-ce/issues/29773