Skip to content
Snippets Groups Projects

operations: Return proper error codes from UserMergeBranch

Merged Patrick Steinhardt requested to merge pks-operations-merge-errors into master
3 unresolved threads

When updating the target reference fails in the UserMergeBranch RPC, then we return an error which is embedded in the response message instead of returning a proper gRPC error with an error code. This behaviour was ported over from the old Ruby implementation, but it's not considered good practice. What makes the problem worse though is that in case git-update-ref(1) fails because of a racy update, then we neither return a gRPC error nor do we set an error message in the response. As a result, the caller can only discern this case by checking whether the response is empty.

Fix this issue by returning proper error codes in all cases and deprecating the in-struct error message.

Fixes #3582 (closed)

Edited by Patrick Steinhardt

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
  • Pavlo Strokov
  • 105 var updateRefError updateref.Error
    106
    107 if errors.As(err, &preReceiveError) {
    108 err = stream.Send(&gitalypb.UserMergeBranchResponse{
    109 PreReceiveError: preReceiveError.Message,
    110 })
    111 } else if errors.As(err, &updateRefError) {
    112 // When an error happens updating the reference, e.g. because of a race
    113 // with another update, then Ruby code didn't send an error but just an
    114 // empty response.
    115 err = stream.Send(&gitalypb.UserMergeBranchResponse{})
    106 if errors.As(err, &updateref.PreReceiveError{}) {
    107 // PreReceiveErrors indicate that any of the hooks rejected the update. This
    108 // typically indicates that the caller tried to update a reference in a way
    109 // that is prohibited.
    110 return status.Error(codes.PermissionDenied, err.Error())
    • Should't we use codes.FailedPrecondition here instead and codes.Aborted when there is a updateref.Error{} error?

    • Question: @8bitlife @pks-t I've just discovered that we've just added an additional error handling for merge conflicts in https://gitlab.com/gitlab-org/gitaly/blob/780588a55b9219f3157cc984f7e1b7aa9f9124f2/internal/gitaly/service/operations/merge.go#L83.

      Failed to merge for source_sha %s into target_sha %s" doesn't seem to say that it was failed due to a merge conflict and it uses codes.FailedPrecondition. Unless we use codes.FailedPrecondition exclusively for merge conflicts in the context of UserMergeBranch, above error message doesn't seem to be clear enough. I'd like to avoid yet another string match to add that information from Rails' side.

      @8bitlife Was there any reason the language of due to conflicts was dropped from the change you've made in the above change? I'd think we could've just kept that so that we can just display that in the UI. Am I missing something? :thinking:

      I feel PreReceiveError would have similar issue since we are dropping preReceiveError value and returning an error code with a message instead. I'm not sure if we can directly map that to Gitlab::Git::PreReceiveError from Rails' side unless codes.PermissionDenied is used only for that purpose.

      It's unclear to me if we're expecting Rails' to display all errors to UI as is except the ones with codes.Internal. If that's the case, I think we need to update the error message for merge conflicts as I mentioned above.

      Please let me know if I missed anything obvious as I'm not very familiar with this part of our codebase. :sweat_smile:

    • I feel PreReceiveError would have similar issue since we are dropping preReceiveError value and returning an error code with a message instead. I'm not sure if we can directly map that to Gitlab::Git::PreReceiveError from Rails' side unless codes.PermissionDenied is used only for that purpose.

      @dskim_gitlab For now I don't think we use codes.PermissionDenied anywhere else, so we should be good here. Sometimes I wish that we invoked /internal/allowed not via HTTP, but via gRPC, too. In that case, we could just take a hands-off approach and directly return the error from Rails including the error code that Rails has set. This would fix the issue we have right now, where the original error gets raised in Rails, then converted by Gitaly and ultimately ends up in Rails again, but now in a different form.

      It's unclear to me if we're expecting Rails' to display all errors to UI as is except the ones with codes.Internal. If that's the case, I think we need to update the error message for merge conflicts as I mentioned above.

      Honestly, I'm not really familiar with how errors are handled by Rails either. Might be worth it to pull in a domain expert in Rails? Maybe @proglottis knows something here?

    • @dskim_gitlab @pks-t Sorry. I'm not sure why I didn't see these mentions earlier.

      The way errors are handled in rails is a mixed bag, spread out all over the place. So it wouldn't surprise me if we're giving the user too much or too little on a case by case basis. The reason PreReceiveError are shown to users is because it has it's own sanitation code, it requires a special "safe" prefix https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/git/pre_receive_error.rb#L13-14

      Maybe using special prefixes will work here too but we really shouldn't rely on plaintext errors from gitaly. They will likely never be localised correctly so displaying them to the user should be a last ditch effort, not to mention the possibility of leaking sensitive data.

      I think especially input validation errors should probably be checked by ruby before the RPC call, or otherwise maybe we need to somehow introduce a structured error that could be interpreted better and run through i18n.

    • @proglottis Yeah, I agree that we would probably want to run it through i18n so it won't be ideal to just display how it was provided from Gitaly. I saw that special safe prefix as well, but didn't seem to be used in many places.

      I wonder if Gitaly should return more precise git error_code instead of plain text error messages? I feel that would be easier to parse from Rails and have proper i18n text for it. I was hoping we could somehow use GRPC error codes for that. However, it doesn't seem that the available set of error types are not enough to cover all cases.

      I'm not sure if it's possible to raise an exception with additional field though. If not, should we add error_codes as a prefix to the error message instead?

      Edited by Sincheol (David) Kim
    • @proglottis @dskim_gitlab

      I'm not sure if it's possible to raise an exception with additional field though. If not, should we add error_codes as a prefix to the error message instead?

      I don't think it is.

      In this context we've got two categories of errors:

      1. Any error which happens in Gitaly itself, or which happens when POSTing to /internal/allowed. These shouldn't ever be printed to the user.
      2. Any error message returned by GitLab's access checks after we have successfully POSTed to /internal/allowed. These are fully under Rails' control, and Gitaly should pass them through without touching the error message.

      In Gitaly, we already prefix the second case with the "GitLab: " safe-prefix, but indeed we also do so for some errors which shouldn't be visible to the user. This needs fixing in Gitaly such that we only do so in case we've got a message from Rails.

      If we fix handling of this prefix in Gitaly to add it if and only if the error message is from Rails, would that help?

    • @pks-t Yeah. If gitaly consistently used a prefix to signify "safe error to give to user" then that'd be much easier to handle.

      In this case I think we still need a permission denied handler on the rails side to provide a generic permission denied error to the user assuming we find no special prefix.

    • @pks-t I agree consistent behaviour would help working with gitaly errors to avoid confusions and possible disclosure of internal error messages.

      Are we using GitLab: prefix only for the error messages that came from GitLab itself as in /internal/allowed as in it means the error message GitLab gave to gitaly?

      I'm talking more about the kind of error messages provided by gitaly itself like merge conflicts. I believe that's handled in https://gitlab.com/gitlab-org/gitaly/blob/780588a55b9219f3157cc984f7e1b7aa9f9124f2/internal/gitaly/service/operations/merge.go#L83 so this MR may not be directly related to that. However, I wonder how we should handle this from Rails side reliably if we can't safely infer that it was indeed a merge conflict nor can we display the error message as is due to the possible safety concern of its content.

      I feel it'd be nice if gitaly can return machine friendly error codes which Rails can map to appropriate error messages with proper i18n support. Having an error prefix could be an alternative solution. Something like ERROR:merge_conflict: Failed to merge for source_sha %s into target_sha %s. It'd be cumbersome to catch all GRPC exceptions and matching error messages though. It seems more appealing to have it return empty response with a particular error_code now instead of having to rescue all those exceptions and inspecting error messages. :thinking:

      In this case I think we still need a permission denied handler on the rails side to provide a generic permission denied error to the user assuming we find no special prefix.

      @proglottis Yes, I think we need additional changes for this MR to work if we're going ahead with this change. I've hijacked the conversation in this MR as it didn't seem to solve the issue I was looking into. :sweat_smile:

    • Are we using GitLab: prefix only for the error messages that came from GitLab itself as in /internal/allowed as in it means the error message GitLab gave to gitaly?

      @dskim_gitlab That's at least the intent, even though it's not currently implmented like this.

      I'm talking more about the kind of error messages provided by gitaly itself like merge conflicts. I believe that's handled in https://gitlab.com/gitlab-org/gitaly/blob/780588a55b9219f3157cc984f7e1b7aa9f9124f2/internal/gitaly/service/operations/merge.go#L83 so this MR may not be directly related to that. However, I wonder how we should handle this from Rails side reliably if we can't safely infer that it was indeed a merge conflict nor can we display the error message as is due to the possible safety concern of its content.

      Right. It really shows that the gRPC errors alone are not a good fit. As @proglottis said before, what we really need are structured errors such that we can tell Rails what exactly has gone wrong, ideally via gRPC messages which are embedded in the error itself.

      There is for example the errdetails package, which allows us to append additional info to errors in a semi-structured way. It would still require parsing of strings though and is thus fragile.

      But more generally, the gRPC error model allows for attaching arbitrary message types to gRPC errors. That looks like a much better fit to me in this context, where we can define RPC-specific error gRPC messages with structured data. In the context of UserMergeBranch, we can for example use the following errors:

      message AccessCheckError {
          message Change {
              bytes old, new, ref;
          }
      
          bytes message [];
          repeated Change changes;
      }
      
      message MergeConflictError {
          bytes ours, theirs, base;
          repeated bytes conflicting_paths;
      }

      On Rails side, you'd have to check whether error details contain any such structs and then generate error messages from them.

      One limitation is that we must restrict error sizes to 2kB at max according to the documentation given that errors are embedded in the header. But it's still a lot better to what we have right now, I'd say.

      Edited by Patrick Steinhardt
    • But more generally, the gRPC error model allows for attaching arbitrary message types to gRPC errors. That looks like a much better fit to me in this context, where we can define RPC-specific error gRPC messages with structured data. In the context of UserMergeBranch, we can for example use the following errors:

      Ok, that might be better indeed if we can send structured data along with the error. I wonder how well that would work if all several gRPC error types could potentially have different kind of messages tagged with them though :thinking:

      Anyways, I'd love to try it out. Would you be able to create an WIP MR for that so that I could test it out from Rails?

    • I wonder how well that would work if all several gRPC error types could potentially have different kind of messages tagged with them though.

      @dskim_gitlab Yeah, this is something I had been wondering, too, it might get unwieldy on Rails side. I thus wondered whether we should instead introduce a per-RPC errror type. Something like the following:

      message UserMergeBranchError {
          enum Type {
              PRE_RECEIVE_ERROR = 1;
              MERGE_CONFLICT = 2;
          };
          message MergeConflictError {
              bytes ours = 1
              bytes theirs = 2
              bytes base = 3;
              repeated bytes conflicting_files = 4;
          };
          message PreReceiveErorr {
              bytes message = 1;
          };
      
          Type type = 1;
          MergeConflictError merge_conflict = 2;
          PreReceiveError pre_receive = 3;
      }

      It's a lot more unwieldy to define, especially so given that quite a lot of these will be repeated across different RPCs. But on Rails side, it's easier to handle given that you know which RPC may return which error details and aren't left guessing. WDYT?

      Edited by Patrick Steinhardt
    • Anyways, I'd love to try it out. Would you be able to create an WIP MR for that so that I could test it out from Rails?

      Yup, I'll implement an RFC.

    • We might not even need a type, but you can probably just try to see which of the sub-messages are set.

    • It's a lot more unwieldy to define, especially so given that quite a lot of these will be repeated across different RPCs. But on Rails side, it's easier to handle given that you know which RPC may return which error details and aren't left guessing. WDYT?

      I think having clearly defined list of possible errors would be very helpful. I feel some kind of error_code defined as enum with a list of possible errors and human a readable error message could be enough? I'm not sure if we are going to need those extra data for generating error message to the end user. :thinking:

    • I feel some kind of error_code defined as enum with a list of possible errors and human a readable error message could be enough?

      @dskim_gitlab I don't think Gitaly should be in a position to generate any message at all, except of course the error message we already generate right now. But these are rather intended for developers such that we can easily find the location where it's generated via breadcrumbs.

      For Rails/users, my plan is not to generate any messages at all in Gitaly, but instead to give Rails the information needed to generate messages. E.g. with above structs, if you see a MergeConflictError, then you can easily generate a message by taking the conflicting files from that error as well as the conflicting versions.

      Edited by Patrick Steinhardt
    • Yes, by error message I meant debuggable message intended for developer as we currently do if that helps. I think receiving some kind of error type would be enough and the extra information would be nice, but I wonder if that makes things more complicated if they are different per error type.

    • @dskim_gitlab @8bitlife @stanhu @zj-gitlab I have reworked this MR to implement above as a proof of concept. Instead of embedding PreReceiveErrors in the response, we make use of the richer gRPC error model which allows us to pass arbitrary error details in the status proto. In the context of UserMergeBranch, we thus have a new UserMergeBranchError message which embeds (for now) only a generic AccessCheckError message:

      // AccessCheckError is an error returned by GitLab's `/internal/allowed`
      // endpoint.
      message AccessCheckError {
        // ErrorMessage is the error message as returned by the endpoint.
        string error_message = 1;
        // Protocol is the protocol used.
        string protocol = 2;
        // UserId is the user ID as which changes had been pushed.
        string user_id = 3;
        // Changes is the set of changes which have failed the access check.
        bytes changes = 4;
      }
      
      // UserMergeBranchError includes error descriptions which may be set as error
      // details in case UserMergeBranch fails.
      message UserMergeBranchError {
        // AccessCheckError is set if the RPC failed because `/internal/allowed` failed.
        AccessCheckError access_check = 1;
      }

      The AccessCheckError is intended to be shared across different RPCs for each RPC which performs access checks, while UserMergeBranchErrors are only used by UserMergeBranch. This allows us to have generic handling for specific errors while making it easy to discover which errors a specific RPC can return. With multiple errors being available, Rails can then check which of the fields is set to deduce the correct error.

      For now, this is still quite limited given that we only treat access check errors. As I said above, we'll want to extend this to also support for example MergeConflictErrors which tell Rails for example which merge failed and what files have conflicts. I wanted to keep it simple for now though to make it easier to iterate if there's any design concerns.

      Let me know what you thing.

      Edited by Patrick Steinhardt
    • There still seems to be a problem with Praefect tests, but I'll figure them out at a later point. Shouldn't be all that hard (famous last words).

    • Thanks a lot for that @pks-t ! I'll play with it tomorrow to see how it looks from the ruby side, but it looks reasonable to me.

      What do you think about adding error_type as an enum though? It'd be ideal if error_type returns AccessCheckError message itself, but I'm not sure if that's possible in go. I think it might be easier to inspect when we have several error types as UserMergerBranchError as you just have to check the error_type once and check the message as opposed to checking all error messages subsequently until you find a match. :thinking:

    • What do you think about adding error_type as an enum though?

      @dskim_gitlab I was torn on whether to add it or not. Ultimately the enum will need to be per RPC as RPCs may have errors which are specific to it, so a single global enum doesn't really work. But if we have per-RPC enums, then it's not a whole lot better to use compared to just checking whether fields are set.

      It'd be ideal if error_type returns AccessCheckError message itself, but I'm not sure if that's possible in go.

      This is not specific to Go, but to gRPC/protobuf.

      Maybe we would be better served if we make the error messages a oneof? That more or less is exactly what we're doing, which is that we return one of a set of different error details. I'm not sure whether Ruby-side tooling makes them enjoyable to use, or if an enum would still be better in that case. Judging by the documentation, it doesn't look like it would significantly help though.

    • I like the proposal of using AccessCheckError.

      oneof looks like it could be a good solution to ensure one and only one error response struct is sent. Using enum could mean we can send multiple error responses.

    • @pks-t Yes, oneof sounds like what we're looking for. It guarantees that there is only one error message type and it gives us the symbol which we can then use to fetch retrieve the data we need.

    • @dskim_gitlab Changed to a oneof now :thumbsup:

    • Thanks @pks-t :pray:

      I'm not sure if I'm doing something wrong, but this is all I get when I raise that error from gitaly.

      Screen_Shot_2021-08-18_at_5.01.28_pm

      I was expecting more structured hash, but I can't find anything else available from https://github.com/grpc/grpc/blob/master/src/ruby/lib/grpc/errors.rb :thinking:

    • @dskim_gitlab The status you've got has the details field set to a Google::Protobuf::Any. You'll need to unmarshal the field to extract the structured error, where its type us UserMergeBranchError.

    • Thanks @pks-t! I can see that I can get error object if I do this.

      Screen_Shot_2021-08-20_at_3.55.53_pm

      I was expecting this was decoded as part of GRPC::PermissionDenied above somehow.

      Does this mean we need to check e.to_rpc_status.details.first[:type_url] for any GRPC error, since UserMergeBranchError could be embedded with any GRPC error, and see if it has matching string of UserMergeBranchError and then decode it to get the structured data from it?

      Also, details seems to be an array so I wonder what should happen if we have more than one item in there :thinking:

      I hope I'm missing something, but it seems a little too cumbersome to me. :sweat_smile:

      @stanhu @proglottis Do you have any thoughts on this? I guess we should move forward with it if this is the best we can do, but I'm unsure if we're making more complicated than it could be as I have very limited knowledge around this. I personally fill like attaching this kind of message to an empty response seems more appealing than utilising GRPC error like this though, but it may not be the best practice.

    • Does this mean we need to check e.to_rpc_status.details.first[:type_url] for any GRPC error, since UserMergeBranchError could be embedded with any GRPC error, and see if it has matching string of UserMergeBranchError and then decode it to get the structured data from it?

      @dskim_gitlab Not sure about Ruby, but in Go you have nice helper functions which know to unpack the type correctly via any.UnmarshalTo.

      I'm unsure if we're making more complicated than it could be as I have very limited knowledge around this. I personally fill like attaching this kind of message to an empty response seems more appealing than utilising GRPC error like this though, but it may not be the best practice.

      The issue with this is that it makes us blind in Praefect: we cannot see whether RPCs have failed or not, and this knowledge is essential to us in some places. The only alternative would be to put RPC-specific knowledge into Praefect, and that's definitely something we want to avoid.

      So even though this may be complicated, I think it is the only feasible way to go ahead.

    • Thanks @pks-t Yes, that makes sense. I suppose it shouldn't be a problem once we lay the foundation with this approach.

      Can you also confirm if details can actually have multiple entries and what do you think we need to do if we have more than one?

    • Can you also confirm if details can actually have multiple entries and what do you think we need to do if we have more than one?

      @dskim_gitlab Yes, they can, but I do not plan on making any use of this in Gitaly. With my current design, each RPC would have at most one error detail, where the error detail is then specific to the RPC at hand (e.g. UserMergeError). So while it is a theoretical issue for now only, you might want to implement some tooling which iterates through all error details until it finds an expected detail and then returns that expected detail. All the other details should just be ignored.

    • Please register or sign in to reply
  • Stan Hu removed review request for @stanhu

    removed review request for @stanhu

  • mentioned in issue #3582 (closed)

  • Pavlo Strokov removed review request for @8bitlife

    removed review request for @8bitlife

    • @pks-t I've just pushed up a draft MR to handle FailedPrecondition errors as it seems include merge conflict related errors which I'm mainly interested in as well as a couple other cases. gitlab!67489 (closed)

      Does that makes sense to you? I suppose we can also include codes.PermissionDenied as well if that's known to be safe.

      I'd like to see if we can update the text I mentioned above to make it clear that there was a conflict as FailedPrecondition doesn't seem to be only raised for merge conflicts. I've chosen to go with Gitlab::Git::CommitError as that sounds like it could include merge conflicts. I was thinking about creating Gitlab::Git::ConflictError, but I don't think we should do that unless gitaly gives us a clear indication that it is a 100% merge conflict.

      @proglottis Do you have any thoughts on this changes?

    • I'd like to see if we can update the text I mentioned above to make it clear that there was a conflict as FailedPrecondition doesn't seem to be only raised for merge conflicts.

      @dskim_gitlab Eventually, I want to also introduce a ConflictError type which we can return from the RPC so that you can get that information. I wanted to focus this MR on the simpler conversion for access checks as a proof of concept for now though. So I'll probably put up another iteration where we just keep the current conflict handling, where the error is then embedded into the normal response. If we agree on the general approach, then I'll iterate and do the ConflictError thing so you can be sure to have a conflict.

    • @pks-t Yes, I agree we should keep this simple so let's add ConflictError after this. I'm not sure if we need another iteration though. Should go straightaway introduce ConflictError? :thinking:

    • I'm not sure if we need another iteration though.

      @dskim_gitlab Not sure if we're talking about the same thing when talking about "another iteration". My intent is to land this MR here with only the changed access checks error handling, and then after this has landed I'll go and create another MR which also changes the way we handle conflict errors. Background is mostly that in order to be able to return a proper ConflictError, Gitaly would need to figure out what's conflicting in the first place. And we don't yet have that info.

      Edited by Patrick Steinhardt
    • So I'll probably put up another iteration where we just keep the current conflict handling, where the error is then embedded into the normal response.

      @pks-t I thought by above you meant we'll 2 more MRs to get us to ConflictError. Maybe I misunderstood :smile:

      Anyways, let's get this one properly reviewed by domain experts and move on. I'll work on handing this from the ruby side :thumbsup:

    • Anyways, let's get this one properly reviewed by domain experts and move on. I'll work on handing this from the ruby side

      @dskim_gitlab Agreed. I'll try to get the MR into a workable state either today or tomorrow, which will require some additional changes on Praefects side. And then we should be ready to go, I think.

    • @dskim_gitlab Should be ready now. Praefect failing was really only an invalid test setup: errors get passed through correctly now.

      @toon, @samihiltunen Could you both please handle the review of this MR?

    • Please register or sign in to reply
  • mentioned in merge request gitlab!67489 (closed)

  • Sami Hiltunen removed review request for @samihiltunen

    removed review request for @samihiltunen

  • Patrick Steinhardt added 209 commits

    added 209 commits

    • 78e738fb...f6ebdc9e - 201 commits from branch master
    • be703472 - hook: Fix prereceive returning AllowedError for generic errors
    • cfc3c395 - updateref: Generalize `PreReceiveError` according to its usage
    • cde1de48 - operations: Allow passing options to `setupOperationsService()`
    • 47e3666c - gitlab: Generalize GitLab mock client
    • 75bf4832 - operations: Adjust names of merge-related tests
    • 9080b136 - helper: Provide error wrappers for `PermissionDenied` codes
    • 46755182 - helper: Implement helper to create gRPC errors with details
    • 106299cc - operations: Implement rich errors for UserMergeBranch access checks

    Compare with previous version

  • added 14 commits

    • 106299cc...1a92a69a - 6 commits from branch master
    • 01e59fa8 - hook: Fix prereceive returning AllowedError for generic errors
    • 5873c46d - updateref: Generalize `PreReceiveError` according to its usage
    • 2c700e03 - operations: Allow passing options to `setupOperationsService()`
    • 0a2944f2 - gitlab: Generalize GitLab mock client
    • 9d21a667 - operations: Adjust names of merge-related tests
    • ec0e233c - helper: Provide error wrappers for `PermissionDenied` codes
    • db5c2168 - helper: Implement helper to create gRPC errors with details
    • dcf4c7b7 - operations: Implement rich errors for UserMergeBranch access checks

    Compare with previous version

  • Patrick Steinhardt changed milestone to %14.3

    changed milestone to %14.3

  • added 84 commits

    • dcf4c7b7...bea500b3 - 75 commits from branch master
    • d4b1ada0 - hook: Fix prereceive returning AllowedError for generic errors
    • 07d9d286 - updateref: Generalize `PreReceiveError` according to its usage
    • 04c3b097 - operations: Allow passing options to `setupOperationsService()`
    • b45bad8f - gitlab: Generalize GitLab mock client
    • a1ea380c - helper: Provide error wrappers for `PermissionDenied` codes
    • 800e8ad6 - helper: Implement helper to create gRPC errors with details
    • b50282c1 - operations: Adjust names of merge-related tests
    • 6ef15409 - operations: Add test exercising UserMergeBranch and access errors
    • 499798ce - operations: Implement rich errors for UserMergeBranch access checks

    Compare with previous version

  • Patrick Steinhardt marked this merge request as ready

    marked this merge request as ready

  • requested review from @toon and @samihiltunen

  • added 23 commits

    • 499798ce...a0d550e5 - 14 commits from branch master
    • 5b9c0721 - hook: Fix prereceive returning AllowedError for generic errors
    • 504b4199 - updateref: Generalize `PreReceiveError` according to its usage
    • f6b94dc8 - operations: Allow passing options to `setupOperationsService()`
    • b393c55d - gitlab: Generalize GitLab mock client
    • 0892f793 - helper: Provide error wrappers for `PermissionDenied` codes
    • 618f89a0 - helper: Implement helper to create gRPC errors with details
    • 5d42b16d - operations: Adjust names of merge-related tests
    • ed2df874 - operations: Add test exercising UserMergeBranch and access errors
    • db627667 - operations: Implement rich errors for UserMergeBranch access checks

    Compare with previous version

  • Rebased to fix conflicts.

  • Toon Claes
  • added 13 commits

    • db627667...32b44d3c - 4 commits from branch master
    • d1b4a1c3 - hook: Fix prereceive returning AllowedError for generic errors
    • 448ca89a - updateref: Generalize `PreReceiveError` according to its usage
    • 5cc68021 - operations: Allow passing options to `setupOperationsService()`
    • eec99771 - gitlab: Generalize GitLab mock client
    • a2387d25 - helper: Provide error wrappers for `PermissionDenied` codes
    • a9a14137 - helper: Implement helper to create gRPC errors with details
    • 99b5794a - operations: Adjust names of merge-related tests
    • 10b27473 - operations: Add test exercising UserMergeBranch and access errors
    • 125d2baf - operations: Implement rich errors for UserMergeBranch access checks

    Compare with previous version

  • Toon Claes approved this merge request

    approved this merge request

  • Toon Claes removed review request for @toon

    removed review request for @toon

  • added 28 commits

    • 125d2baf...dec431ea - 19 commits from branch master
    • 773668f5 - hook: Fix prereceive returning AllowedError for generic errors
    • c6821337 - updateref: Generalize `PreReceiveError` according to its usage
    • 0b2a982c - operations: Allow passing options to `setupOperationsService()`
    • f34bed05 - gitlab: Generalize GitLab mock client
    • 7fee3c93 - helper: Provide error wrappers for `PermissionDenied` codes
    • be0671b8 - helper: Implement helper to create gRPC errors with details
    • f4c36e35 - operations: Adjust names of merge-related tests
    • 2afa4ccd - operations: Add test exercising UserMergeBranch and access errors
    • e542a7d8 - operations: Implement rich errors for UserMergeBranch access checks

    Compare with previous version

  • Rebased to fix conflicts.

  • Sami Hiltunen approved this merge request

    approved this merge request

  • Sami Hiltunen enabled an automatic merge when the pipeline for 535a5e57 succeeds

    enabled an automatic merge when the pipeline for 535a5e57 succeeds

  • merged

  • Sami Hiltunen mentioned in commit c8311c7d

    mentioned in commit c8311c7d

  • added workflowcanary label and removed workflowstaging label

  • added workflowproduction label and removed workflowcanary label

  • Patrick Steinhardt mentioned in merge request !3851 (merged)

    mentioned in merge request !3851 (merged)

  • mentioned in merge request gitlab!70357 (merged)

  • mentioned in issue #3845

  • Patrick Steinhardt mentioned in merge request !4120 (merged)

    mentioned in merge request !4120 (merged)

  • Due to an issue the workflowstaging-ref label was incorrectly applied to this merge request. Re-setting back to workflowproduction using https://gitlab.com/gitlab-org/release-tools/-/merge_requests/1620

  • mentioned in issue gitlab#321358

  • Please register or sign in to reply
    Loading