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

State install works without a project #1421

Merged
merged 33 commits into from
Jul 14, 2021
Merged

Conversation

MDrakos
Copy link
Member

@MDrakos MDrakos commented Jun 17, 2021

@MDrakos MDrakos requested a review from Naatan June 18, 2021 18:47
Copy link
Member

@Naatan Naatan left a comment

Choose a reason for hiding this comment

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

@MDrakos since you're going on vacation don't worry about addressing these comments, I'll get on this myself since it's unlikely we'd wrap this up today.

Comment on lines 44 to 52
language, err = model.LanguageForPackage(params.Package.Name())
if err != nil {
return locale.WrapError(err, "err_install_get_langauge", "Could not get language for package: {{.V0}}", params.Package.Name())
}
} else {
language, err = model.LanguageForCommit(a.proj.CommitUUID())
if err != nil {
return locale.WrapError(err, "err_fetch_languages")
}
Copy link
Member

Choose a reason for hiding this comment

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

This is fundamentally flawed as we should not have any opinion on the language, that's what the solver is for.

Of course that does raise issues for certain State Tool use-cases, but we should be addressing those issues rather than working around them by spoofing the language.

Comment on lines 51 to 53
if pj == nil {
return installInitial(cfg, out, authentication, prompt, packageName, packageVersion, languageName, operation, ns)
}
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't make sense that we would just skip all the normal operations in favor of this special behavior. I think what we need to do at this point is create the project file, and then the rest of the logic can run as it normally would.

@@ -162,3 +171,57 @@ func getSuggestions(ns model.Namespace, name string) ([]string, error) {

return suggestions, nil
}

func installInitial(cfg configurable, out output.Outputer, authentication *authentication.Auth, prompt prompt.Prompter, packageName, packageVersion, languageName string, operation model.Operation, ns model.Namespace) error {
Copy link
Member

Choose a reason for hiding this comment

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

fwiw not reviewing this function as I don't believe it should exist as per my comment above.

@@ -85,6 +86,35 @@ func LanguageForCommit(commitID strfmt.UUID) (string, error) {
return languages[0].Name, nil
}

// LanguageForPackage attempts to fetch the language belonging to the given package name
func LanguageForPackage(name string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't exist, that's what the solver is for.

@MDrakos
Copy link
Member Author

MDrakos commented Jul 6, 2021

The failing integration test is unrelated to this PR. Follow up story followed here: https://www.pivotaltracker.com/story/show/178788386

@MDrakos MDrakos requested a review from Naatan July 6, 2021 18:50
Naatan
Naatan previously approved these changes Jul 14, 2021
# Conflicts:
#	internal/runners/packages/install.go
#	internal/runners/packages/packages.go
#	internal/runners/packages/uninstall.go
@Naatan Naatan merged commit 8141af1 into master Jul 14, 2021
@Naatan Naatan deleted the install-no-project-178223554 branch July 14, 2021 22:36
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.

2 participants