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

Remove .gitignore from ignore files in up #481

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

Milo123459
Copy link
Collaborator

This removes the searching of .gitignore in the up command. This previously caused some unknown behaviour.

@Milo123459 Milo123459 added the release/minor Author minor release label Feb 15, 2024
Copy link

Hello, @Milo123459! Thanks for your submission.

Our team will respond soon. If you need more immediate help, try our Forum or our Discord. Thanks!

Copy link
Contributor

@zuchka zuchka left a comment

Choose a reason for hiding this comment

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

lgtm!

@Milo123459 Milo123459 merged commit 03db6ac into master Feb 15, 2024
8 checks passed
@Milo123459 Milo123459 deleted the milo/remove-gitignore-from-up branch February 15, 2024 23:03
@brody192
Copy link
Contributor

brody192 commented Feb 15, 2024

I can see this causing far more issues than it fixes, including but not limited to local .env files making it into the build and container. Please keep in mind railway up is not used only from a github action where the files have already been ignored.

@aleksrutins
Copy link
Contributor

I agree with Brody, this will break anybody's builds who's trying to use railway up to deploy a Node.js app from a Windows machine.

@Milo123459
Copy link
Collaborator Author

I agree with Brody, this will break anybody's builds who's trying to use railway up to deploy a Node.js app from a Windows machine.

why? node_modules is ignored by default.

@maddsua
Copy link
Contributor

maddsua commented Feb 19, 2024

This is what having .gitignore included makes me do in my workflows. Just gonna leave it here.
pic

@aleksrutins
Copy link
Contributor

This is what having .gitignore included makes me do in my workflows. Just gonna leave it here.

deno task build should be something you run in the Railway builder, no? The recommended workflow (as I understand it) is just to check if you can deploy it in your CI file, and then Railway's CI does the actual building.

As a compromise, though, could we add perhaps a --include-gitignored flag (or similar) to achieve this functionality without making it mandatory?

@brody192
Copy link
Contributor

brody192 commented Feb 19, 2024

Changing the default behavior that has been around since early v0 of the cli is not the best idea, node_modules may be hard coded to be ignored, but this change will break other things in other unimaginable and subtle ways, for the people who need the cli to ignore the .gitignore, a simple flag to ignore the .gitignore would be a perfect solution.

@aleksrutins
Copy link
Contributor

Also, in order to include specific files, (as I implemented it in v2) you can add a .railwayignore with ! lines. Not 100% sure if that works in v3, though.

@brody192
Copy link
Contributor

brody192 commented Feb 19, 2024

I think the idea could work, but the .gitignore file is last to be added in v3, unlike in v2 where .railwayignore came last.

@maddsua
Copy link
Contributor

maddsua commented Feb 19, 2024

deno task build should be something you run in the Railway builder, no? The recommended workflow (as I understand it) is just to check if you can deploy it in your CI file, and then Railway's CI does the actual building.

Not in this case, railway wouldn't be able to build this project because of the way it's structured (a monorepo that is not approved by the lizard govt). My point isn't in about the build step but in the fact that I need to move that service to a temp directory which is not covered by gitignore in order for railway to be able to see all the needed files

@maddsua
Copy link
Contributor

maddsua commented Feb 19, 2024

Well if we can't change the default behavior, why not just add an option to disable gitignore? Something like --no-gitignore wouldn't hurt anybody

@brody192
Copy link
Contributor

I am in favor of leaving the default behavior and adding a --no-gitignore flag

@zuchka
Copy link
Contributor

zuchka commented Feb 20, 2024

--no-gitignore is a non-breaking change, and that's ideal. Seems like a good compromise

@Milo123459
Copy link
Collaborator Author

#484 has been made

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community release/minor Author minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants