Skip to content
Snippets Groups Projects

Add Iteration Cadences to Group Import/Export

Merged George Koltsov requested to merge georgekoltsov/add-iterations-to-import-export into master
1 unresolved thread

What does this MR do and why?

This MR adds iterations cadences & iterations to Group Import/Export, so when the group is migrated, the group iteration cadences are preserved.

The migrated cadences will then be used (in the future MR) to preserve issue iteration infromation, burndown charts, etc.

Mentions #291983 (closed)

Screenshots or screen recordings

cadences

How to set up and validate locally

  1. Create a group with iteration cadence & cadences
  2. Export the group using Group Export functionality (under Settings -> Advanced)
  3. Download exported tarball and import it back
  4. Verify imported group has group iteration cadences & iterations imported and they are identical to the source group

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by George Koltsov

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
36 - :duration_in_weeks
37 - :iterations_in_advance
38 - :active
39 - :automatic
40 - :roll_over
41 - :title
42 - :description
43 iterations_cadences: *iterations_cadence_definition
44 iteration: &iteration_definition
45 - :iid
46 - :created_at
47 - :updated_at
48 - :start_date
49 - :due_date
50 - :group_id
51 - :description
  • Comment on lines +45 to +51

    Question: I'm not familiar with iterations. Why aren't we exporting the other fields?

    cached_markdown_version
    title
    state_enum
    sequence
    title_html
    description_html
  • @rodrigo.tomonari forgot to mention about these fields:

    1. cached_markdown_version is a prohibited field so theres no point in exporting it https://gitlab.com/gitlab-org/gitlab/blob/georgekoltsov%2Fadd-iterations-to-import-export/lib/gitlab/import_export/attribute_cleaner.rb#L15-15 same for *_html fields.
    2. title of iteration will become a deprecated field. I read that in the documentation https://docs.gitlab.com/ee/user/group/iterations/ After Iteration Cadences becomes generally available, manual iteration scheduling will be deprecated in GitLab 15.6. To enhance the role of iterations as time boundaries, we will also deprecate the title field.
    3. state_enum is recalculated in before save hook, so I left this field out https://gitlab.com/gitlab-org/gitlab/blob/georgekoltsov%2Fadd-iterations-to-import-export/ee/app/models/ee/iteration.rb#L56-56
    4. sequence - I didn't notice this field before. I'll add it :thumbsup:
  • Edited by George Koltsov
  • Looking at the SQL that is ran there we might need to preserve sequence and not run the after_save hook :thinking: not sure. I'll leave sequence out for now.

    Edited by George Koltsov
  • @euko I think you worked on this. I assume sequence is useful to export since it captures how many times the iteration was updated?

  • I will merge this for now since he is out of office.

  • Contributor

    @stanhu

    Yes sequence is a useful piece of information and should be exported.


    For iterations cadence, the following fields don't need to be preserved during import/export in my opinion:

        - :last_run_date
        - :duration_in_weeks
        - :iterations_in_advance
        - :roll_over

    When automatic is set true, it indicates automatic scheduling should be enabled and a worker creates new iteration objects for the cadence according to the fields. I think a user can re-enable the automation after importing a cadence appropriately. What do you think @gweaver?

    @georgekoltsov if we omit them in import_export.yml, what values do they get in the db when imported again?

    Issue: #371472

  • if we omit them in import_export.yml, what values do they get in the db when imported again?

    @euko These values will be nil when importing. I see that roll_over has not null constraint, but the rest can be excluded if we want. I can create a new MR, if needed.

    Can you please clarify on automatic - are you suggesting we do not migrate automatic field either?

    I will also add a new MR to add sequence, thanks for clarification on it.

  • These values will be nil when importing. I see that roll_over has not null constraint, but the rest can be excluded if we want. I can create a new MR, if needed.

    That said if there are ActiveRecord callbacks that update these fields, they will be updated

  • @euko also wondering if this callback should be ran for updating sequence?

  • Contributor

    @georgekoltsov

    These values will be nil when importing. I see that roll_over has not null constraint, but the rest can be excluded if we want. I can create a new MR, if needed.

    Can you please clarify on automatic - are you suggesting we do not migrate automatic field either?

    Yes I am suggesting we don't migrate automatic field and set them to false on import by default. I would like to get an additional opinion from @gweaver on this though.

    To confirm, when automatic is false, roll_over should be defaulted to false as well. The rest can be null: last_run_date, duration_in_weeks, iterations_in_advance incl. start_date.

    (note to myself: I should consider adding check constraints that mirror the model validations i.e., certain fields must be non-null when automatic is true.)

    also wondering if this callback should be ran for updating sequence?

    Running the callback should not be necessary to update sequence on import unless they've been tempered with during export/import.

  • Yes I am suggesting we don't migrate automatic field and set them to false on import by default.

    @euko is there a reason why we wouldn't migrate the automated settings? If there is a technical challenge, then I'm fine with not doing this. I also see value in potentially including these from a user experience perspective.

  • Contributor

    I also see value in potentially including these from a user experience perspective.

    @gweaver There is no technical challenge. I was thinking a user would re-enable automatic scheduling for (like power-on) a cadence if it's really actively used after an import and the rest of the cadences can stop generating new iterations. I don't have a strong opinion here. It's up to you.

  • Thanks @gweaver @euko so which approach should we go with?

  • @euko @georgekoltsov lets go with your intuition on this one and not migrate the automated settings. This is a two way door so we can always iterate based on feedback :smile:

  • Please register or sign in to reply
  • George Koltsov added 1 commit

    added 1 commit

    • aaa6a5ed - Add Iteration Cadences to Group Import/Export

    Compare with previous version

  • George Koltsov added 1 commit

    added 1 commit

    • 92d433cc - Add Iteration Cadences to Group Import/Export

    Compare with previous version

  • Confirming that changes validate locally :thumbsup_tone1:

  • Carla Drago approved this merge request

    approved this merge request

  • Carla Drago removed review request for @carlad-gl

    removed review request for @carlad-gl

  • :wave: @carlad-gl, thanks for approving this merge request.

    This is the first time the merge request is approved. To ensure full test coverage, a new pipeline has been started.

    For more info, please refer to the following links:

  • Rodrigo Tomonari approved this merge request

    approved this merge request

  • Rodrigo Tomonari removed review request for @rodrigo.tomonari

    removed review request for @rodrigo.tomonari

  • @stanhu may I ask you to review this one, please? :pray:

  • George Koltsov requested review from @stanhu

    requested review from @stanhu

  • Stan Hu approved this merge request

    approved this merge request

  • Stan Hu resolved all threads

    resolved all threads

  • merged

  • @stanhu, did you forget to run a pipeline before you merged this work? Based on our code review process, if the latest pipeline finished more than 2 hours ago, you should:

    1. Ensure the merge request is not in Draft status.
    2. Start a pipeline (especially important for Community contribution merge requests).
    3. Set the merge request to merge when pipeline succeeds.

    This is a guideline, not a rule. Please consider replying to this comment for transparency.

    This message was generated automatically. You're welcome to improve it.

  • Stan Hu mentioned in commit 49ec240a

    mentioned in commit 49ec240a

  • added workflowstaging label and removed workflowcanary label

  • mentioned in issue #371462 (closed)

  • Stan Hu mentioned in commit 94d4c1ea

    mentioned in commit 94d4c1ea

  • Stan Hu mentioned in commit 7872735a

    mentioned in commit 7872735a

  • Stan Hu mentioned in merge request !95872 (merged)

    mentioned in merge request !95872 (merged)

  • Stan Hu mentioned in commit 87d4f8fd

    mentioned in commit 87d4f8fd

  • Stan Hu mentioned in commit 9c8d88bb

    mentioned in commit 9c8d88bb

  • Stan Hu mentioned in commit 99a43156

    mentioned in commit 99a43156

  • Stan Hu mentioned in commit 19596726

    mentioned in commit 19596726

  • George Koltsov mentioned in merge request !95918 (merged)

    mentioned in merge request !95918 (merged)

  • 🤖 GitLab Bot 🤖 added groupimport and integrate label and removed 1 deleted label

    added groupimport and integrate label and removed 1 deleted label

  • Please register or sign in to reply
    Loading