Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

minio: set version to a valid datetime string #121786

Merged
merged 1 commit into from May 11, 2021
Merged

Conversation

jojosch
Copy link
Member

@jojosch jojosch commented May 5, 2021

Motivation for this change

Minio verifies on each RPC call that the server version returned by the API is a valid datetime string (https://github.com/minio/minio/blob/3a0e7347cad25c60b2e51ff3194588b34d9e424c/browser/app/js/web.js#L51-L53).

When returning the version as "2021-04-22T15-44-28Z" this check fails and the login (and all other RPC calls) in the minio browser are not possible:

image

Is there a cleaner way to replace just the last two dashes in "2021-04-22T15-44-28Z" with colons so that the version attribute can be left as-is and only the part in the patchPhase will be adjusted?

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@06kellyjac
Copy link
Member

06kellyjac commented May 5, 2021

{ lib, buildGoModule, fetchFromGitHub, nixosTests }:

let
  modifyTimestamp = version:
    let
      splitTS = builtins.elemAt (builtins.split "(.*)(T.*)" version) 1;
    in
    builtins.concatStringsSep "" [ (builtins.elemAt splitTS 0) (builtins.replaceStrings [ "-" ] [ ":" ] (builtins.elemAt splitTS 1)) ];
in
buildGoModule rec {
  pname = "minio";
  version = "2021-04-22T15-44-28Z";

  src = fetchFromGitHub {
    owner = "minio";
    repo = "minio";
    rev = "RELEASE.${version}";
    sha256 = "147a4vgf2hdpbndska443axzvxx56bmc0011m3cq4ca1vm783k8q";
  };

  vendorSha256 = "0qj1zab97q8s5gy7a304wqi832y8m083cnk8hllz8lz9yjcw6q92";

  doCheck = false;

  subPackages = [ "." ];

  patchPhase = ''
    sed -i "s/Version.*/Version = \"${modifyTimestamp version}\"/g" cmd/build-constants.go
    grep "Version." cmd/build-constants.go
    sed -i "s/ReleaseTag.*/ReleaseTag = \"RELEASE.${version}\"/g" cmd/build-constants.go
    sed -i "s/CommitID.*/CommitID = \"${src.rev}\"/g" cmd/build-constants.go
  '';

  # ...
}

I came up with this function but it's a bit ugly lol

grep "Version." cmd/build-constants.go result:

 * Licensed under the Apache License, Version = "2021-04-22T15:44:28Z"
        // Version = "2021-04-22T15:44:28Z"
        Version = "2021-04-22T15:44:28Z"

@r-rmcgibbo
Copy link

r-rmcgibbo commented May 5, 2021

Result of nixpkgs-review pr 121786 at 8d9165ed run on aarch64-linux 1

1 package built successfully:
  • minio
1 suggestion:
  • warning: patch-phase

    patchPhase should not be overridden, use postPatch instead.

    Near pkgs/servers/minio/default.nix:25:3:

       |
    25 |   patchPhase = ''
       |   ^
    

Result of nixpkgs-review pr 121786 at 8d9165ed run on x86_64-linux 1

1 package built successfully:
  • minio
1 suggestion:
  • warning: patch-phase

    patchPhase should not be overridden, use postPatch instead.

    Near pkgs/servers/minio/default.nix:25:3:

       |
    25 |   patchPhase = ''
       |   ^
    

@bachp
Copy link
Member

bachp commented May 10, 2021

@ryantm will this cause issues with the automatic update if the version doesn't match the tag? If yes I prefer the solution from @06kellyjac

pkgs/servers/minio/default.nix Outdated Show resolved Hide resolved
pkgs/servers/minio/default.nix Outdated Show resolved Hide resolved
pkgs/servers/minio/default.nix Outdated Show resolved Hide resolved
@SuperSandro2000
Copy link
Member

@ryantm will this cause issues with the automatic update if the version doesn't match the tag? If yes I prefer the solution from @06kellyjac

Yeah it will. It would require a custom update script which is not necessary.

@ryantm
Copy link
Member

ryantm commented May 10, 2021

@bachp You can add "nixpkgs-update: no auto update" to a comment in the expression, or ask me to add it to the Skiplist if you don't want nixpkgs-update to touch it.

@jojosch
Copy link
Member Author

jojosch commented May 11, 2021

I've added the solution by @06kellyjac. With that we get continued compatibility with the auto update and the minio browser works.

@bachp
Copy link
Member

bachp commented May 11, 2021

@ryantm No I would very much like to keep auto updates, they are very handy. With the updated solution they should continue to work.

@06kellyjac
Copy link
Member

Adding the following to the bottom of your commit would be appreciated but not required 🙂

Co-authored-by: 06kellyjac <dev@j-k.io>

https://docs.github.com/en/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors

@ofborg ofborg bot requested a review from bachp May 11, 2021 10:14
Co-authored-by: 06kellyjac <dev@j-k.io>
@bachp bachp merged commit c58155c into NixOS:master May 11, 2021
@06kellyjac
Copy link
Member

06kellyjac commented May 11, 2021

👍 🎉

@jojosch jojosch deleted the minio-version branch May 11, 2021 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants