Update product adapter for changes in the UserService

[isacikgoz]

Summary

Make changes to the adapter WRT mattermost/mattermost#20674

[agnivade]

Ahh my bad. I saw this, but I thought it's the app/user user service.

[isacikgoz]

@mgdelacroix lint is not failing on my local setup, I checked out to userCtx branch on every project. It's failing on the CI but I think should've done the same right? Do you have any idea why it can be failing? cc/ @pfltdv

[agnivade]

I see. Focalboard CI is not setup to fetch branches from the server and enterprise. We'd need to force merge this, and work on setting the CI.

[agnivade]

@mgdelacroix - Just to clarify - how is the boards multi-product architecture working without the CI testing it along with the server and enterprise repo cloned with it? Shouldn't it have been done with #3381 itself?

[ghost]

@mgdelacroix lint is not failing on my local setup, I checked out to userCtx branch on every project. It's failing on the CI but I think should've done the same right? Do you have any idea why it can be failing? cc/ @pfltdv

@isacikgoz @mgdelacroix It is strange that; it fails with below reason and build succeeds. Must likely a pipeline dependency version issue or caching issue. I will try to reproduce. Strange CI failure reason is below:

Error: product/api_adapter.go:37:29: too many arguments in call to request.EmptyContext
	have (mlog.LoggerIFace)
	want () (typecheck)
		ctx: request.EmptyContext(api.logger),
		                          ^
Error: product/api_adapter.go:97:60: too many arguments in call to a.api.userService.UpdateUser
	have (*"github.com/mattermost/mattermost-server/v6/app/request".Context, *"github.com/mattermost/mattermost-server/v6/model".User, bool)
	want (*"github.com/mattermost/mattermost-server/v6/model".User, bool) (typecheck)
	user, appErr := a.api.userService.UpdateUser(a.ctx, user, true)

[agnivade]

@pfltdv - Can you point me to the part in the build pipeline where it fetches the same branch names from server and ee repos? Something must be there in the build pipeline which is not in the lint pipeline.

[ghost]

@pfltdv - Can you point me to the part in the build pipeline where it fetches the same branch names from server and ee repos? Something must be there in the build pipeline which is not in the lint pipeline.

@agnivade lint has different pipeline. Pipeline executes only make server-lint.It is located here. https://github.com/mattermost/focalboard/blob/main/.github/workflows/lint-server.yml#L15-L22

[agnivade]

Yes, so comparing with this: https://github.com/mattermost/focalboard/blob/main/.github/workflows/ci.yml, I don't see anywhere where the server and ee repos are checked out. So how is the build pipeline passing, but this one failing?

[ghost]

Yes that is the strange part, still investigating the reason.

[ghost]

Mattermost server has this commit 8 days ago. mattermost/mattermost@6dc897b which is not part of any tags.

Playbooks has mattermost server dependency indirectly. Also i reproduced that failure in vscode locally (without updating mattermost-server).

Looks like dependency issue in go.mod file.

[agnivade]

The code is fine. You have to clone the userCtx branch of focalboard, server and enterprise and setup the go.work correctly for all of this to work. But I want to know how come the build pipeline is passing, but lint is failing. How is the build pipeline taking the userCtx branches of server and ee repos?

[isacikgoz]

@pfltdv could we also fetch mattermost-server while runing th linter? I think it would fix the issue. Or alternatively, we can merge the server PR then update the server dependency. It can also be another fix.

[ghost]

I am not sure about updating mattermost-server or cloning will solve the problem source cause lint action execute below actions:

cd server; golangci-lint run ./...
cd mattermost-plugin; golangci-lint run ./...

Both have same dependencies, so server lint should fail also. And it only fails for mattermost-plugin.

[ghost]

There is no clone for build so all dependencies managed by go. And honestly i do not know how branches are selected, it uses commit hashes internally.

And for linting needs we should not depend on other project source codes. Specially should not clone our private project in public repository. Maybe we should review our codebase cause outside contributors can not build this project due to lintin issues.

Is it possible to make server change released first, then update dependencies of this project. So we will depend on a released code of mattermost-server?

[agnivade]

@pfltdv - I'll have a separate discussion with the release team on how the multi-product architecture works and what to do. Essentially now 3 repos are just a single repo compiled together.

Specially should not clone our private project in public repository.

We already do that in the server.

And honestly i do not know how branches are selected, it uses commit hashes internally.

Yes, it uses the hash to determine whether there's a branch to clone. This is how:

            if [[ "<< pipeline.parameters.tbs_sha >>" == "" ]]
            then
              git clone --depth=1 --no-single-branch https://github.com/mattermost/mattermost-server.git
              cd mattermost-server || exit
              git checkout $CIRCLE_BRANCH || git checkout master
            else
              git clone --depth=1 --no-single-branch https://github.com/<< pipeline.parameters.tbs_server_owner >>/mattermost-server.git
              cd mattermost-server || exit
              git checkout << pipeline.parameters.tbs_server_branch >>
            fi
            git config --global url."git@github.com:".insteadOf "https://github.com/"

https://github.com/mattermost/enterprise/blob/e468443eb8dae1fa32c9b6a1bde0946db2336110/.circleci/config.yml#L77-L86

@mgdelacroix / @wiggin77 - I am wondering if the cloning of server and ee branches are happening somewhere that isn't super apparent in the build pipeline? If not, we should definitely get that set up first to get the compilation working.

[wiggin77]

I am wondering if the cloning of server and ee branches are happening somewhere

@agnivade I've not looked into this yet. Not sure who would maintain that part of CI automation, and if it turns out I need to do it, I might as well set it up for Playbooks as well.

[agnivade]

@wiggin77 - Understood, yes we need to do it. Anything in the circleci config files is owned by us. The Gitlab pipelines are owned by the release team.

Re: playbooks, I am not sure if there's a benefit of doing that since we aren't ready for that yet.

[ghost]

PR is created to clone enterprise repo for lint pipeline. #3418 .

[ghost]

#3418 is merged into main.

[isacikgoz]

/update-branch

[ghost]

@agnivade @isacikgoz Since we need to include some repositories for some pipelines to work we introduced environment variables and changed go.work.

At this file https://github.com/mattermost/focalboard/blob/fd9b62e0d60bf40db880f02e54ed09205d0202a5/mattermost-plugin/build/gowork/main.go we are configuring CI differently that block us to use local checked-out repos in CI. Do we really need to configure CI environment different than local environment ?

Changes are here: fd9b62e

[agnivade]

Thank you @pfltdv ! I am now unblocked ❤️

Re: the question, I'll defer to @wiggin77. I'll wait till I have a final green light from Doug on this.

[wiggin77]

Do we really need to configure CI environment different than local environment ?

The naming (local versus CI) can be improved, but the issue is that Boards has multiple SKU's, one being a stand-alone build that runs for a local user. The different SKUs require different go.work. As I recall, I tried including all the repos that could be needed in go.work, but when a use directory is missing is causes errors in most Go commands, even if the directory won't be imported.

@pfltdv . @agnivade I'm open to any suggestions to make this better.

[agnivade]

Ah so it's a naming problem. Wondering if we should rename ci to standalone if that makes more sense? 0/5

[agnivade]

I am going to merge this to unblock myself. Happy to send another PR to improve the naming.

[ghost]

Thanks alot @wiggin77 for clarification , naming is not that important. We were just curious about having different behavior at different systems. Since we will apply similar logic to mattermost-server and enterprise pipelines. For those pipelines we will keep this in mind.

[wiggin77]

@pfltdv @agnivade this PR broke the dev environment for Boards developers. Anyone who didn't already have a go.work created before the PR ends up with a file without the mattermost-server include. People need to know that a USE_LOCAL_MATTERMOST-SERVER_REPO env var must be created.

I'm not entirely clear what the requirements are for CI, but I would like to have the default (no env var) do the right thing, and have a CI script declare a var if needed to change behaviour.

[agnivade]

Sounds good. So the mattermost-server include needs to be by default. I'll defer to Mustafa but no issues from my side.

[ghost]

@wiggin77 @agnivade We will include it by default and exclude it on ci. However that is not the only environment variable configured for different ci systems. Does anyone knows that why we need to include ./linux folder for ci systems ?

https://github.com/mattermost/focalboard/blob/main/mattermost-plugin/build/gowork/main.go#L62-L64

https://github.com/mattermost/focalboard/blob/main/mattermost-plugin/build/gowork/main.go#L70-L83

[wiggin77]

We will include it by default and exclude it on ci.

I am working on a PR to fix this. Banging on the open-graph / pluginapi issue then I'll submit and tag you @pfltdv as a reviewer in case I missed anything.

[ghost]

Thanks alot @wiggin77 i am working on fixing mattermost-server issue.

[ghost]

Related PR, #3516. I will request reviews after fixing all pipelines.

[wiggin77]

Does anyone knows that why we need to include ./linux folder for ci systems ?

CI builds and checks multiple SKU's for boards, including a stand-alone server. That stand-alone server is built via the linux folder.