backstage / backstage

Backstage is an open framework for building developer portals
https://backstage.io/
Apache License 2.0
26.89k stars 5.58k forks source link

πŸ› Bug Report: Multiple Merge Request creation to same Gitlab Repo fail with 400 Bad request #16662

Open sharadpattanshetti opened 1 year ago

sharadpattanshetti commented 1 year ago

πŸ“œ Description

Hi, We are using the action: publish:gitlab:merge-request in our Merge Request templates and generating the MR to an existing repo in Gitlab. First MR will be successful, however any subsequent MR with same branch name or different branch name fails with 400 Bad request error.

Please let me know how can this be resolved?

πŸ‘ Expected behavior

The template should generate subsequent MRs successfully as long as the branch name is new.

πŸ‘Ž Actual Behavior with Screenshots

The publish action fails with below error.

Commit to branch failed

image

πŸ‘Ÿ Reproduction steps

  1. Create a software template with publish step using - action: publish:gitlab:merge-request
  2. Try to publish the MR to an existing Gitlab repo. Once successful, merge the MR and close it.
  3. Try to create another MR to the same repo, it fails with above error.

πŸ“ƒ Provide the context for the Bug.

No response

πŸ–₯️ Your Environment

No response

πŸ‘€ Have you spent some time to check if this bug has been raised before?

🏒 Have you read the Code of Conduct?

Are you willing to submit PR?

None

sharadpattanshetti commented 1 year ago

Tried to debug the code to check the branch name passed to the commit action as in below snippet. Even though branch name is new and non-existing but still commit fails.

ctx.logger.info('repoID' +repoID + 'branchName' + branchName + 'ctx.input.title' + ctx.input.title ); await api.Commits.create(repoID, branchName, ctx.input.title, actions);

Rugvip commented 1 year ago

@sharadpattanshetti perhaps there's a bit more information available on the error object from the GitLab API if we log it without stringifying? I think the best way to figure out what's going on here is to narrow down the error that we're getting back from GitLab.

sharadpattanshetti commented 1 year ago

Hi @Rugvip, Just tried to debug little more by referring to the gitbreaker docs and tried to print e.description which shows below error:

error: Commit to branch failed A file with this name already exists

Way our template is structured is as below: Step - 1. Merge Request Template clones the main (also called as Template Repo) repo using cookiecutter action. This repo has the files in a sub directory (ex. terraform) inside ${{cookiecutter.project_slug}} folder. Step - 2. Creates a Merge Request to another target repo using action: publish:gitlab:merge-request

What is Working: If the target repo does not have the same directory or files (under same hierarchy) then the MR is successful.

What is Not Working: If the target repo has same folder or file (under same hierarchy) then above error is thrown with 400 Bad Request. The reason we have this scenario is because the target repo had used same MR template for this feature sometime back and now wants to get updated version of it from the template repository.

Hope this helps in understanding the issue, please let us know if there is any workaround and better solution ?

Rugvip commented 1 year ago

Hmm, there is a commitAction option that you can set to 'update' or 'delete' instead of the default 'create'. That should let you update files rather than create new ones. If you do need something more granular than that I think that might be capabilities that we'd need to add to the GitLab merge action.

Btw, would be awesome if you want to ship the change of forwarding the exact error coming back from GitLab in a PR, that would make troubleshooting easier for everyone! πŸ™

sharadpattanshetti commented 1 year ago

Thanks @Rugvip that really helped me. What I thought is to ask user to select create or update action in template ui depending upon whether the feature already exists in target repo or not. Created 2 publish actions for create and update and conditionally triggering them. We are testing all the scenarios like - change in files, new file added etc for both create and update.

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sergeyshevch commented 1 year ago

@Rugvip I also met with the same issue. Can you please reopen it and assign it to me? I will implement returning of error

It also looks like mixed actions are not supported now. (Create new file and update some old)

sharadpattanshetti commented 1 year ago

The same issue occurring for us when we try to update a file already existing in the repo in Gitlab using this action. What would be the cause for this one?

sergeyshevch commented 1 year ago

@sharadpattanshetti For now this action only can create new files. Diffing and updating of existing aren't supported. You can use git actions for it

sharadpattanshetti commented 1 year ago

Thanks @sergeyshevch. Is this more of a limitation from gitbeaker library we are using? Because we were trying to explore the @isomorphic-git library and most of the actions like branch creation, commit on existing file update etc are working fine.

Rugvip commented 1 year ago

@sharadpattanshetti it's a limitation of the GitLab APIs, which gitbeaker uses. The workaround is to work directly with the git protocol and data, which isomorphic-git does.

sharadpattanshetti commented 1 year ago

Thanks @Rugvip, we tried the same with isomorphic-git but the facing challenge wrt creating clean MR.

oreonl commented 10 months ago

We also have a problem with not being able to do updates and creates on the same MR. Is there any plan to support this?

sergeyshevch commented 10 months ago

@oreonl Currently I'm out of capacity for this issue. Feel free to open PR if you want

Rugvip commented 10 months ago

@sharadpattanshetti could you elaborate a bit more on what you tried?

@oreonl as far as we can tell we'll need to move over to using git operations directly rather than the GitLab APIs. There's currently no capacity to work on this from any of the maintainer teams so we're relying on community contributions.

sharadpattanshetti commented 10 months ago

@Rugvip finally what worked for us was a custom action to create the MR which will actually generate the modified/changed files.

  1. In the action we used the isomorphic-git lib for the git operations like new branch creation, branch checkout, git commit and git push. Also, used Gitlab API to create a MR since isomorphic-git had some issue creating the MR. (all these operations should be in same order)
  2. After branch checkout we have logic to merge the updated folder and files using the @alumna/reflect lib. This lib helps you replace the updated directory contents in the target directory.
  3. This custom publish action syntax is same as the backstage built in action.
  4. It is not that neat because of multiple libs being used but there were problems using one single lib for all the operations.

Hope this helps you, let me know if you have any other questions.

Rugvip commented 10 months ago

@sharadpattanshetti could you elaborate a bit more on what step 2 does?

sharadpattanshetti commented 10 months ago

Hi @Rugvip, I will try to explain, hope it will be helpful : In the 2 step the issue we were facing was that - Initially we used the fs-extra lib's writeFileSync and other functions, the MR is created in the Gitlab it was not showing the diff or the updated changes only. It was showing contents of each file as addition. Hence we chose @alumna/reflect.

The @alumna/reflect lib's reflect method helps us shift all the updated files from the source repo to the destination repo using the below snippet.

let { res, err } = await reflect({ src:${ctx.workspacePath}/${directory}, dest:${templateDir}/${directory}, // (OPTIONAL) Default to 'true' recursive: true, // (OPTIONAL) Default to 'true' // Delete in dest the non-existent files in src delete: true, // (OPTIONAL) // Array with files and folders not to reflect // exclude: ["skip-this-file.txt", "skip/this/directory"] });

This worked beautifully if the source repo has single folder which gets updated. However, if there is more than one folder updated or a new folder added additionally then it is not working. We are looking at fixing that issue.

Please let me know if you have better suggestions on this.

ichasco-heytrade commented 9 months ago

Hi, I have the same error.

Is there any update?

Thanks!!

Rugvip commented 9 months ago

@sharadpattanshetti ah alright, yep that sounds like a good approach for step 2. Beyond that you'll want to use isomorphic-git to do a shallow clone of the existing repo, then you sync over all the files with @alumna/reflect (or similar method). Once the new files are in the target repo you add all files, equivalent of git add --all, create a new branch and commit all changes, then push the new branch to GitLab. At this point you switch over to calling the GitLab API to create a new MR using the branch that you pushed.

sharadpattanshetti commented 9 months ago

Yes exactly @Rugvip. We identify the status of the sub folders and files isomorphic-git lib's status-metrics and then handle modified, delete scenarios. It is bit complicated but able to get almost all scenarios working. Please let me know if you need more info.

tudi2d commented 9 months ago

Thanks @sharadpattanshetti - sounds great. Would you be up for contributing a fix to this bug? :)

sharadpattanshetti commented 9 months ago

sure @tudi2d . Occupied a bit with urgent deliverables now, will try in few days definitely. Before that I wanted your and @Rugvip opinion if this is the best way to achieve that functionality?

jhaals commented 9 months ago

sure @tudi2d . Occupied a bit with urgent deliverables now, will try in few days definitely. Before that I wanted your and @Rugvip opinion if this is the best way to achieve that functionality?

That sounds like a good idea πŸ‘

vdt-mik commented 9 months ago

I faced with the same problem. @sharadpattanshetti Can you share a code snippet for isomorphic-git which you use?

ichasco-heytrade commented 8 months ago

@sharadpattanshetti Is there any update about this? Thanks!

mickfeech commented 8 months ago

Also looking for updates

sharadpattanshetti commented 7 months ago

@Rugvip and everyone sorry for the late reply. Had to check the policies before posting. Hope this helps..

  1. Read integration config and other required details in the handler function
  2. Clone the template repo - this might have happened using cookiecutter action in the first step in template.yaml
  3. Clone the component repo using below command, execSync is from built in nodejs lib :
    require("child_process").execSync(`git clone https://oauth2:${token}@${cloneUrl} ${templateDir} --depth 1`)
  4. Serialize the template directory({{cookiecutter_componentId}}) content as in the publish:gitlab-merge-request action
let fileRoot: string;
            if (sourcePath) {
                fileRoot = resolveSafeChildPath(ctx.workspacePath, sourcePath);
            } else if (targetPath) {
                // for backward compatibility
                fileRoot = resolveSafeChildPath(ctx.workspacePath, targetPath);
            } else {
                fileRoot = ctx.workspacePath;
            }
            const fileContents = await serializeDirectoryContents(fileRoot, {
                gitignore: true,
            });
            const fileContentsIntermediate = await serializeDirectoryContents(intermediateDir, {
                gitignore: true,
            });
  1. Now the fileContents dir has all the serialized template dir content and fileContentsIntermediate is having all the serialized component content(where the merge request is going to be created)
  2. Create local git branch using @isomorphic-git lib
  3. Read complete folder tree from the Repo before doing sync
  const getAllDirectoryNames = fs.readdirSync(ctx.workspacePath, { withFileTypes: true })
                .filter((item) => item.isDirectory())
                .map((item) => item.name);
            const getAllFileNames = fs.readdirSync(ctx.workspacePath, { withFileTypes: true })
                .filter((item) => item.isFile())
                .map((item) => item.name);
            getAllFileNames.map(async (file: any) => {
                fs.copySync(`${ctx.workspacePath}/${file}`, `${templateDir}/${file}`);
            })
  1. Call @alumna/reflect to sync each folder of template repo to the component repo
await Promise.all(
                getAllDirectoryNames.map(async (directory_test) => {
                    await getDirectory(templateDir, ctx.workspacePath, directory_test);
                })
            )
  1. Get modified files using the status using the @isomorphic-git lib
// GET ADDED/MODIFIED FILES  
            await Promise.all(
                fileContents.map(async (file: any) => {
                    let status = await git.status({ fs, dir: `${templateDir}`, filepath: `${file.path}` })
                    if (status === '*added' || status === '*modified' || status === 'absent') {
                        ctx.logger.debug(`my-gitlab-merge-request : Step 2 - FILES ADD/UPDATE: ${file.path}:${status}`)
                        await git.add({ fs, dir: `${templateDir}`, filepath: `${file.path}` })
                    }
                })
            )
  1. Get Deleted file @isomorphic-git lib

    
    //GET DELETED FILES  
            await Promise.all(
                fileContentsIntermediate.map(async (deletedFile: any) => {
                    let status = await git.status({ fs, dir: `${templateDir}`, filepath: `${deletedFile.path}` })
                    if (status === '*deleted') {
                        if (fs.existsSync(`${templateDir}/${deletedFile.path}`)) {
                            await git.remove({ fs, dir: `${templateDir}`, filepath: `${deletedFile.path}` })
                        }
                        else {
                            await fs.copy(`${intermediateDir}/${deletedFile.path}`, `${templateDir}/${deletedFile.path}`);
                            await git.remove({ fs, dir: `${templateDir}`, filepath: `${deletedFile.path}` })
                            ctx.logger.debug(`my-gitlab-merge-request : Step 2 - FILES DELETED: ${deletedFile.path}`)
                        }
    
                    }
                })
            )

11. Do a commit and push using @isomorphic-git lib 
12. Create MR using gitbreaker lib
13. Additionally if you want to create target branch/verify in between these steps etc then use gitbreaker lib 
ichasco-heytrade commented 7 months ago

Any news about this? Thanks!

vinzscam commented 7 months ago

the issue is up for grabs @ichasco-heytrade!

gavlyukovskiy commented 5 months ago

GitLab API supports different actions per file, so the scaffolder action could be modified to make it per file/path, for example:


    - id: gitlab_merge_request
      name: publish:gitlab:merge-request
      action: publish:gitlab:merge-request
      input:
        repoUrl: gitlab.com?repo=backstage-playground&owner=agavlyukovskiy
        title: "Test MR"
        branchName: my-mr-branch
        commitActions:
          - path: CODEOWNERS
            action: 'update'
          - path: my-new-dir
            action: 'create'
          - path: dont-like-dir
            action: 'delete'

instead of current approach where we use the same commitAction for all files

ichasco-heytrade commented 5 months ago

Hi @gavlyukovskiy And is it not possible for this to be done automatically? if the file or directory does not exist, let it be created, if it does exist, let it be updated. In my case, I cannot define a concrete path because it will vary.

gavlyukovskiy commented 5 months ago

@ichasco-heytrade if I understand you correctly, it would require using GitLab API for every file. I guess it's technically possible, but I'd avoid that due to rate limiting.

github-actions[bot] commented 3 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ichasco-heytrade commented 3 months ago

Is there any news?

benjdlambert commented 3 months ago

Want to link this issue here too: #22244

pitzelalex commented 3 months ago

GitLab API supports different actions per file, so the scaffolder action could be modified to make it per file/path, for example:


    - id: gitlab_merge_request
      name: publish:gitlab:merge-request
      action: publish:gitlab:merge-request
      input:
        repoUrl: gitlab.com?repo=backstage-playground&owner=agavlyukovskiy
        title: "Test MR"
        branchName: my-mr-branch
        commitActions:
          - path: CODEOWNERS
            action: 'update'
          - path: my-new-dir
            action: 'create'
          - path: dont-like-dir
            action: 'delete'

instead of current approach where we use the same commitAction for all files

This would absolutely solve the exact issue I'm having. Trying to make a GitLab MR where we're creating a file and updating an existing file at the same time.

mickfeech commented 1 month ago

Is anyone working on this?

jlozano-rh commented 1 week ago

I'm getting the exactly same issue in gitlabRepoPush. There is any update?

caseyw commented 1 week ago

We're also seeing this.

jlozano-rh commented 1 week ago

In my case, using the action gitlab:repo:push I had to specify a separate init file and then the rest of New files in this action. Because when I used commitAction: update apply to all files. Implement next approach would be good for many scenarios:

GitLab API supports different actions per file, so the scaffolder action could be modified to make it per file/path, for example:


    - id: gitlab_merge_request
      name: publish:gitlab:merge-request
      action: publish:gitlab:merge-request
      input:
        repoUrl: gitlab.com?repo=backstage-playground&owner=agavlyukovskiy
        title: "Test MR"
        branchName: my-mr-branch
        commitActions:
          - path: CODEOWNERS
            action: 'update'
          - path: my-new-dir
            action: 'create'
          - path: dont-like-dir
            action: 'delete'

instead of current approach where we use the same commitAction for all files