Release managers working across multiple projects in GitLab often use Group Milestones to collect related items for a scheduled a release. You may have realized that GitLab Releases could be associated to a Project Milestone before and now you can associate a Group Milestone to a Project Release via the Releases API. This will be a great help to all the Release Managers coordinating their releases with milestones.
Only group milestones that belong to the project's immediate group can be added to the existing milestones array, milestones belonging to any of the project's ancestor groups cannot be added. There is existing validation in place that ensures there is not a naming clash between project and project's group milestones.
The signature if the of the release API call does not hange, and the groups could be added to the same array, ie:
@jreporter we may want to investigate if we try to extend the existing Release API endpoints by adding a group / milestone info, or possibly it would make more sense to create a new endpoint.
@sean_carroll - can you create a tech evaluation for:
Investigate if we try to extend the existing Release API endpoints by adding a group / milestone info, or possibly it would make more sense to create a new endpoint.
I added an issue for the GraphQL implementation #235494
Jackie Porterchanged title from Modify the releases API to allow for Group Milestones to be associated with Project Releases to Support Group Milestones to be associated with Project Releases in API
changed title from Modify the releases API to allow for Group Milestones to be associated with Project Releases to Support Group Milestones to be associated with Project Releases in API
Good call out @sean_carroll and thanks for adding it @jreporter! I haven't thought about adding blocking relationships before, but that's definitely a good suggestion especially when we break down issues into smaller "sub-issues."
@sean_carroll thanks, and yes that is true. However @jakeburden and I had this conversation prior to planning, and identified some areas that could start in front of backend work.
@jakeburden I think the best place to call it out would be in our own handbook page for release management, as engineering workflow would apply to all teams.
@sean_carroll - I think this is the primary issue we should be focusing on in %13.5 - it is allowing users to leverage the existing API to add a Group Milestone as a milestone association - I don't think a blocking issueis actually blocking the implementation of this. So I will remove this relationship.
Ran into a blocker / question on the implementation:
When a milestone is added to a project, it cannot already exist on the project's group, or vice versa.
When adding a milestone to the release, we just pass the milestone name.
The problem occurs when the group has a parent group. At this level it is possible to add a milestone that already exists at the project level. See the M1 milestone in this screenshot.
Only allow addition of milestones that belong to either the project or the project's immediate group. This follows existing behavior of milestones with groups and subgroups.
In this case the API call would not be changed, and the groups could be added to the same array, eg:
currently it is possible to add a same-named milestone to a grandparent group and a project. Is this intentional or a bug that should be fixed before we start extending it?
It is possible to add both subordinate milestones to release-group
At the release-subgroup level
It is possible to add a milestone with the same name as on it's parent group, but not possible to add one with the same name as the project level (there is an error message).
If we go with option 1, we cannot add group-milestone to the release, as it belongs to the top level release-group.
Option 2 is to provide Milestone IDs instead of names, and then allow all milestones to be associated with the release
If we take option 2, perhaps the way would be to leave the existing milestones field in place for project milestones only, and then add a new group_milestone_ids field in the API which would allow for any level of group milestones.
Thanks for digging into this @sean_carroll! To be candid about our API, in the past I have thought that it would be nicer to pass IDs rather than names to avoid naming collisions (and it makes things a bit cleaner on the frontend .) But we can address that in a separate issue at another time.
I love the thinking here, and I think my only questions are:
With Option 1, when we make a GET request for a release, will that release then have all milestones, including any group milestones, in the single milestones array?
If we go with Option 1 now, and iterate to Option 2 in the future, would we still return all milestones in the milestones array, or would group_milestones be returned as their own array? And if it's the latter, would it be technically feasible to retroactively separate group milestones from project milestones for releases that have already been associated with group milestones while Option 1 was enabled?
Going with Option 1, this would be my plan for the frontend:
Populate the milestone dropdown combobox on the Release edit/new page with Group milestones - Option 1 would also make this change simpler, though I've already merged some changes that makes Option 2 work here (not a problem since we're working iteratively.) By going with Option 1, when a user selects either a project milestone or group milestone, the name of the milestone gets added to a milestones array and is then POST to the backend when the release is created or updated. The potential bug here is if the user selects a project milestone and group milestone that share the same name.
If we go with option 1, we cannot add group-milestone to the release, as it belongs to the top level release-group.
Option 2 is to provide Milestone IDs instead of names, and then allow all milestones to be associated with the release
If we take option 2, perhaps the way would be to leave the existing milestones field in place for project milestones only, and then add a new group_milestone_ids field in the API which would allow for any level of group milestones.
I am satisfied by option 1, we should create follow up issues to implement milestone ids @sean_carroll
The advantage of leaving the API interface as-is under option 1 is there are no breaking changes. I'll create a follow-up issue and we can pass ids to a new field group_milestone_ids.
will that release then have all milestones, including any group milestones, in the single milestones array?
Yes it should work this way.
technically feasible to retroactively separate group milestones from project milestones for releases that have already been associated with group milestone
It's actually just the API interface - all the milestones are associated in the same table milestone_releases in the backend.
It's actually just the API interface - all the milestones are associated in the same table milestone_releases in the backend.
Should we consider if we want to split this into separate tables some time in the future (in a different issue?)
From an architectural point of view, I wonder if there might be long term advantages to being able to query group_milestone_releases and project_milestone_releases as individual entities. Definitely not right now, but would that give us an advantage for features on the horizon?
Feel free to let me know if this seems like a non-issue, it's more of a question for my curiosity
We had a discussion with @sean_carroll and this topic came up.
I see one other option here: can we just add single milestone_ids array to the API? (note that group/project milestones can't have the same ID)
It doesn't require us to remove anything since we adding a new parameter. But we can deprecate current milestones in favor of milestone_ids, we don't need to wait to %14.0 since we won't break anything.
It removes any "group/supergroup/project" ambiguity. (with current implementation we don't allow supergroup milestones to be used, so they should be filtered out on the frontend or we should handle errors properly. This problem will just go away if use ids)
I assume it will be easier for frontend, @jakeburden?
There will be no group_milestones/project_milestones, just a single simple parameter.
IMHO, it feels much easier to add this parameter right away
I understand that we're already in the end of the development cycle, but I think it will be easier to change the direction now than write a few workarounds and create follow-up issues.
Thanks for the thoughts @vshushlin, this is a variation of Option 2 as described above.
It's interesting that fresh eyes are coming back to this approach. Apart from where we are in the dev cycle, WDYT @jakeburden about having a single array of IDs (both project and group milestones), ie: going back to Option 2.
One difference from the current approach is the ID is passed, instead of the milestone name, ie: an integer like 123 vs M1.
@sean_carroll@vshushlin A single array of IDs here works for me. I have an MR almost ready for review that updates the state management for release milestones on the frontend, making this change easy enough to make when it's ready.
@vshushlin from the API perspective, adding the milestone_ids fields (passing an array of milestone ids) would be certainly possible and does not effect the existing milestones array which can be left alone or deprecated, as you have noted.
What also needs to be added is validation that there is a relationship between the milestone and the release. I understand this as being: the release belongs to the same project or group as the milestone. Looking from the perspective of the milestone, this relationship could be one of these:
milestone's project = release.project this is the current state in master
milestone belongs to a group that contains a project which = release.project current version of this MR
milestone belongs to a group that through any of it's subgroups contains a project which = release.project
To support the third point, we will still need to do the validations (and all the tests) already included in !43385 (merged), add the new parameter to the API and also traverse possible subgroups.
Am I missing something about the needing to enforce a relationship between a milestone and release?
the release belongs to the same project or group as the milestone
@vshushlin@jreporter just to be 100% clear - is this statement correct? The release must be connectable to the milestone through a group and eventually via a project?
If it is, then I can't see how we can avoid the validations even when directly adding the milestone IDs.
@sean_carroll@vshushlin - I agree that milestone id will be the most exact way to validate the association. Release tags will always be associated to a project since that is where the code lives. The only validation I could see wanted to be added is if the project belongs to the group for which the group milestone exists.
@jreporter I agree with this, so in this case I see the current piece of work as a stepping stone to the goal off being able to associate any milestones from higher level groups also.
Issue status: 80% complete, 75% confidentMR statuses:https://gitlab.com/gitlab-org/gitlab/-/merge_requests/44157 - 80% complete - basic tests were missing for existing milestone functionality. Needs 4 reviews but am not expecting big changes.https://gitlab.com/gitlab-org/gitlab/-/merge_requests/44395 - 0% complete, 100% confident - API documentation update (not started, but will be simple)https://gitlab.com/gitlab-org/gitlab/-/merge_requests/43385 - 75% complete, 75% confident - functionality working OK when all code is in CE, but moving to EE posed some problems.
The code was prototyped in CE and works OK when CE-only, but there is a blocker with this issue related with the GitLab metaprogramming that separates the code into EE:
When extending the app/services/releases/concerns.rb module Releases::Concerns into EE, there are loading conflicts. There are some notes on the MR.
@nicolewilliams I will continue to investigate solutions to these problems and reach out to other engineers for support if it cannot be resolved soon.
As part of this MR we need to offer different behaviour for CC and EE users. Specifically the project_group_id is passed to the MilestonesFinder but can only contain a value if EE.
# overridden in EEdefproject_group_id# project.group&.idend
Looking into the extension of Releases::Concerns, which is found in the services directory as app/services/releases/concerns.rb, a few thoughts:
In this MR we are trying to extend this module, which is an ActiveSupport::Concern, overriding an 'empty' method with code only in EE.
There is a concerns folder under app/services where most of the 'concerns' are contained, although some of these are traditional Ruby modules and are not extending ActiveSupport::Concern
The naming of the module as Releases::Concerns is slightly problematic, although not a technical problem, a concern is generally named as an adjective. Releases::Concerns seems a bit vague.
There seem to be some issues with extending a module which extends ActiveSupport::Concern
The create, update and destroy services all mix in the Releases::Concerns module. Perhaps it would be better to have a superclass, which could be more easily extended in EE.
More information about prepending EE code onto ActiveSupport::Concern:
Yeah, that seems much more obvious to me (as someone who doesn't know much about these services). It's much easier to override (in EE) a method in a class rather than a module.
# TODO: New services should consider inheriting from# BaseContainerService, or create new base class:# https://gitlab.com/gitlab-org/gitlab/-/issues/216672
fwiw, #273173 contains a request to allow milestones deeper down the group list and there exists a corresponding patch (but without updating/implementing spec/ tests).