@vespian_gl, thanks for the latest updates and hard work here.
@vespian_gl and I paired on end-to-end testing this today. A small hotfix seems to still be required to deal with an empty attribute which likely needs a default. @vespian_gl's looking into that, and will push updates soon. I don't expect this to require drastic changes to the implementation. But let me know if you find any blockers, and I can help you figure out if there is an easy solution.
Besides, the code looks good in general to me. I'm happy to approve this one. The only blocking piece to merge this MR in my opinion would be:
For this I suggest we follow the same chart e2e test we conducted:
This is the last required action from my point of view. I'll leave this thread open for us to address this.
pipeline fix
I really like the conventional commits usage. But unfortunately this project currently has a hard requirement on The commit subject must start with a capital letter.
Could you please amend the commits which start with lower case on their subject (titles)?
This should fix https://gitlab.com/gitlab-org/omnibus-gitlab/-/jobs/13506312395.
Yes. I'd prefer to generalize where possible, and reuse existing features.
Sounds good. I don't have a strong opinion on this one. Happy to keep just deleting the files.
I agree with your reasoning.
But yes, I'd prefer if we change the behaviour in an isolated piece of work. This way it's easier to rollback or even just identify that a certain behaivour change caused some unexpected outcome. We likely want to release on a separate milestone.
Thanks for reverting the behaviour.
total hours spent: 22h
@mbursi, I think that because the workflow-infraIn Progress was added on Thursday, as seen from this epic's history, and the bot message update for the operate team is configured for Tuesday, the bot message had already passed for last week. So any Epic that starts to get tracked on the Operate top level epic after Tuesday, will only get the comment by the bot asking to update on the following week.
But the fact that this epic is present in the top level epic description, already tells me that the automation is well set up and in place. The other parts which were already configured were,
---
<!-- STATUS NOTE START -->
<!-- STATUS NOTE END -->
Thank you so much, @vespian_gl.
This looks very much inline with what we discussed.
Still I've left a few notes for your considerations. Nothing is a major change. Just typos, a few questions, and some non-blocking thoughts.
Please, kindly have a look. We can also discuss these items in our sync next week, as this MR is a priority over the chart refactoring.
question
This is interesting. If I understand correctly, it's limiting the backup restore user to only be able to connect to the GitLab managed PostgreSQL database via a socket (local), and using md5.
Do we need to also implement a host <%= @registry['database']['dbname'] %> <%= @registry['database_backup_username'] %> <CIDR> md5 in case Rails is on a separate node than the GitLab managed PostgreSQL?
Or does some of the automations on this file already handle that?
Or our current legacy backup-restore tooling currently requires to connect via a socket, so there's currently already a tight-coupling between PostgreSQL and the Rails nodes?