Skip to content

Rename rojo project during upload when wally package name doesn't match #25

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Sep 29, 2021
Merged

Rename rojo project during upload when wally package name doesn't match #25

merged 6 commits into from
Sep 29, 2021

Conversation

magnalite
Copy link
Contributor

@magnalite magnalite commented Aug 23, 2021 •
edited
Loading

At the moment it is possible to publish a package which has a name differing from a name given in a Rojo default.project.json. This means Rojo will rename the package later on causing problems with linking. This PR aims to mitigate this by preventing users publishing packages with mismatched names and prompting them to rename.

It does so by checking if a default.project.json file exists in the project during publishing, and if it does, asserts the project name matches the package name given in the wally.toml.

This PR also adds:

  • A test to verify publishing will stop when names are mismatched (and users are given an appropriate error).
  • Another test to verify users are given an appropriate error when the names are fine (instead of a mismatched error).
  • An extra global argument to allow tests to specify a temporary package index should be used. This caused problems when multiple publish tests were being run.
  • The publish command now also accepts global arguments.
  • Renames the wally-test-index url in the test packages to the new one (instead of the old archived one which 404s).

@magnalite magnalite requested a review from LPGhatguy August 23, 2021 21:29
@magnalite magnalite linked an issue Aug 23, 2021 that may be closed by this pull request
Package name in wally.toml doesn't match name in project.json #14
Closed
@LPGhatguy
Copy link
Contributor

There are some good changes in this PR! I think that we should shift the titular feature. Instead of validating that the user's default.project.json matches, we should just fix it for them in the zip that we upload. This is important because we can't or don't want to change the default.project.json for many projects because it impacts the name of the model when built by Rojo, like for standalone releases!

@magnalite
Copy link
Contributor Author

There are some good changes in this PR! I think that we should shift the titular feature. Instead of validating that the user's default.project.json matches, we should just fix it for them in the zip that we upload. This is important because we can't or don't want to change the default.project.json for many projects because it impacts the name of the model when built by Rojo, like for standalone releases!

Sounds like a good idea! I'll change this to modify the default.project.json during upload. I think we should still output a warning indicating that this is happening and recommend changing it to match if possible. Obviously that warning will not stop the upload, it'll just clarify what is happening.

@LPGhatguy
Copy link
Contributor

I think we should still output a warning indicating that this is happening and recommend changing it to match if possible.

I don't know about this. For Roact, they probably want to keep the project name as Roact because this is important for rojo build. Changing the name to roact would be desirable for Wally but would break the normal releases.

I think we can have an info-level log message, but we shouldn't necessarily push people to change their Rojo projects.

@magnalite
Copy link
Contributor Author

magnalite commented Sep 8, 2021 •
edited
Loading

I think we can have an info-level log message, but we shouldn't necessarily push people to change their Rojo projects.

Sounds good to me. I've now changed this to detect+modify a default.project.json during creation of PackageContents and output a neutral info log. I'm not sure if this still counts as an integration test and I would have preferred to keep using the publish command but that felt like unnecessary complexity.

@magnalite magnalite changed the title Return appropriate error when rojo project and wally package names don't match Rename rojo project during upload when wally package name doesn't match Sep 8, 2021
@magnalite magnalite merged commit beeb9a8 into main Sep 29, 2021
@magnalite magnalite deleted the rojo-project-handling-soft-fix branch September 29, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LPGhatguy LPGhatguy Awaiting requested review from LPGhatguy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Package name in wally.toml doesn't match name in project.json
2 participants
@magnalite @LPGhatguy