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

Added --remote option #191

Merged
merged 2 commits into from
Jun 10, 2024
Merged

Added --remote option #191

merged 2 commits into from
Jun 10, 2024

Conversation

colinvh0
Copy link
Contributor

@colinvh0 colinvh0 commented Jun 6, 2024

I don't use origin as my primary remote and I didn't want to use the --repo option every time, so I added a --remote option.

Copy link
Member

@JohannesHoppe JohannesHoppe left a comment

Choose a reason for hiding this comment

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

I think the --repo param has nothing to do with this, has it?
Otherwise LGTM! 🙂👍

@@ -24,6 +24,11 @@ commander
'Provide the repository URL. If no value is provided, the `origin` remote of the current working directory is used.',
defaults.repo
)
.option(
'-R, --remote <remote>',
'Provide the remote name. If no value is provided, and --repo is not set, `origin` is used.',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Provide the remote name. If no value is provided, and --repo is not set, `origin` is used.',
'Provide the remote name. If no value is provided `origin` is used.',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line 145 of engine.ts prevents calling getRemoteUrl if options.repo is set.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but even then the default would be origin. While reading this, my brain struggles to parse this sentence:

  1. If no value is provided --> so we provide nothing?!
  2. and --repo is not set --> AND nothing is set?!
  3. origin is used. ---> only, THEN then it's origin??

But it's always origin. 🤪

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I've understood the source of the confusion: there are two local repos that this software interacts with. If --repo isn't set, it reads the origin remote URL from the repo in the local directory (I'll refer to this as the installed repo), then it clones a repo, presumably in a temp directory. The latter I will refer to as the cloned repo.

I didn't intend to interact with the cloned repo's remote's name, but I now presume that I have. However, I believe it doesn't matter what remote name it uses, as long as it handles the deployment properly, so I think my code is okay there.

My intent was to have the --remote option change which remote URL is read from the installed repo, and I think my code does that.

Copy link
Member

Choose a reason for hiding this comment

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

Wow, I had to read this three times. 😅 How can we make this understandable for a normal person? Maybe a documentation of your use case in the README.md?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're having trouble with anything I write, I'm happy to rephrase it.
Most people have a git directory that they set up with
git remote add origin https://github.etc
They can use this package with
ng deploy
I have
git remote add github https://github.etc
I want to
ng deploy --remote github

Copy link
Contributor Author

@colinvh0 colinvh0 Jun 10, 2024

Choose a reason for hiding this comment

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

Has this helped any in understanding why --repo affects this?

Edited to add:
My documentation was copied from and is still nearly identical to the documentation for --repo:

If no value is provided, the `origin` remote of the current working directory is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/engine/engine.ts line 334 is where this software reads from the installed repo.

I have pushed an update to the --help messages to hopefully make them clearer.

Copy link
Member

Choose a reason for hiding this comment

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

Excellent! I will craft a new release. 🙂🙏

@@ -24,6 +24,10 @@
"default": false,
"description": "Skip build process during deployment."
},
"remote": {
"type": "string",
"description": "Provide the remote name. If no value is provided, and --repo is not set, `origin` is used."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"description": "Provide the remote name. If no value is provided, and --repo is not set, `origin` is used."
"description": "Provide the remote name. If no value is provided `origin` is used."

@JohannesHoppe JohannesHoppe merged commit 30c6b0a into angular-schule:main Jun 10, 2024
@d-koppenhagen
Copy link
Contributor

Just a little question: shouldn't it be release as 2.1.0 since it's a new feature aka minor release?

@JohannesHoppe
Copy link
Member

Hmmm... Yes! 😅

@JohannesHoppe
Copy link
Member

JohannesHoppe commented Oct 23, 2024

@colinvh0 Could you please explain your exact setup? How do you use the --remote option? This change introduced a regression, because it affects the --repo option as well. Are you setting both options? More context would be super helpful to offer a solution that works for everyone. See #193

@colinvh
Copy link

colinvh commented Oct 23, 2024

I use ng deploy --remote github which uses the github remote as configured in the local repo. I don't name my remotes origin unless they're decidedly upstream from me. The idea of this option was to not have to specify a URI with the --repo option, since the local repo is my single source of truth for that URI. (I created the colinvh0 account because I had temporarily lost access to this one.)

@JohannesHoppe
Copy link
Member

Thank you for the details. This will help me to ensure that your use case is sufficiently covered!

FYI @fmalcher

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.

4 participants