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

feat: Add functionality for nix. Fixes #10998 #10999

Merged
merged 12 commits into from
Jun 1, 2023

Conversation

isubasinghe
Copy link
Member

@isubasinghe isubasinghe commented Apr 28, 2023

Please see #10998 for a detailed comment regarding why this is needed.

This is a change from our fork that I ported to the upstream repo because it works quite well.

The steps to run Argo now are quite simple, and guaranteed to work exactly the same on every machine.
Not only this, the entire Argo build system is isolated from the rest of the user's filesystem.

  1. modify the hosts file according to this, don't worry about the other instructions.
  2. setup a k8s cluster, k3d is the recommended solution here.
  3. Install Nix
  4. Run "nix develop" (you will have to use the experimental feature flags on a fresh install, the features you need are "nix-command" and "flakes").
  5. Run "devenv up".

@isubasinghe isubasinghe changed the title Add functionality for nix. Fixes #10998 Feat: Add functionality for nix. Fixes #10998 Apr 28, 2023
@isubasinghe isubasinghe changed the title Feat: Add functionality for nix. Fixes #10998 feat: Add functionality for nix. Fixes #10998 Apr 28, 2023
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of new files at top-level. Is that necessary?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
@isubasinghe
Copy link
Member Author

There are a lot of new files at top-level. Is that necessary?

I believe I can probably refactor away the node-env.nix and node-package.nix by bringing some of that into flake.nix.
The others, I do need them.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
conf.nix Outdated Show resolved Hide resolved
Copy link
Contributor

@juliev0 juliev0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requested changes (inline/comments)

@isubasinghe
Copy link
Member Author

isubasinghe commented May 25, 2023

Sorry @juliev0 and @terrytangyuan for my late engagement on this issue, was quite busy with another bug (#11102).

@isubasinghe
Copy link
Member Author

New changes:

  1. Moved files to "nix-files" directory.
  2. Did not modify README.md
  3. Moved to original staticfiles repo
  4. Documentation on how to upgrade a nix dependency was added.

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just left a minor comment. Thanks!

server/static/files.go Show resolved Hide resolved
@terrytangyuan
Copy link
Member

terrytangyuan commented May 25, 2023

Should we move this to /hack folder instead of creating a dedicated one in root directory?

@isubasinghe
Copy link
Member Author

@terrytangyuan I am happy to move it over if you think that is better, but I personally don't think it belongs there.

The hack folder is specifically reserved for hacks that the Makefile uses, this is entirely separate from that so we shouldn't put it there imo.

Is that alright? I'll chuck it in the hack folder otherwise.
Thanks!

Makefile Show resolved Hide resolved
@terrytangyuan
Copy link
Member

The hack folder is specifically reserved for hacks that the Makefile uses, this is entirely separate from that so we shouldn't put it there IMO.

I feel like a nix-files folder might be overkill. If not in hack, I've seen folders like dev or build for this purpose which might be more suitable. We should move .devcontainer to the same folder as nix-files if dev container allows it to be in a different folder.

@isubasinghe
Copy link
Member Author

@terrytangyuan
Yeah I think a dev folder works in that case, I just wanted to avoid overloading the meaning of the hack folder.

I don't use devcontainers but I can ask someone who does (@Joibel ) to help me out as it does look like devcontainers does support the config in subdirectories: microsoft/vscode-remote-release#2413

Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
@Joibel
Copy link
Member

Joibel commented Jun 1, 2023

The hack folder is specifically reserved for hacks that the Makefile uses, this is entirely separate from that so we shouldn't put it there IMO.

I feel like a nix-files folder might be overkill. If not in hack, I've seen folders like dev or build for this purpose which might be more suitable. We should move .devcontainer to the same folder as nix-files if dev container allows it to be in a different folder.

For autodiscovery of "this project has a devcontainer config" I think .devcontainer needs to stay where it is.

@terrytangyuan terrytangyuan merged commit bccba40 into argoproj:master Jun 1, 2023
22 checks passed
@terrytangyuan
Copy link
Member

@isubasinghe Can you help look into why the build is failing after merge? https://github.com/argoproj/argo-workflows/actions/runs/5145625809/jobs/9263514577

@isubasinghe
Copy link
Member Author

@terrytangyuan

If it is really this PR, the only source of error I can think of is this -> https://github.com/argoproj/argo-workflows/pull/10999/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L1-R1

The only reason I can see is the Makefile changing, but that seems somewhat unlikely given the CI tests ran fine. Might just be a coincidence, looking into it now.

@isubasinghe
Copy link
Member Author

It could also be the files.go actually, will fix ASAP.

@isubasinghe
Copy link
Member Author

@terrytangyuan here you go, it was missing a space :

#11160

JPZ13 pushed a commit to pipekit/argo-workflows that referenced this pull request Jul 4, 2023
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
jeremyhager pushed a commit to jeremyhager/argo-workflows that referenced this pull request Jul 7, 2023
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: Jeremy Hager <47301461+jeremyhager@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants