Skip to content
This repository has been archived by the owner on Apr 13, 2020. It is now read-only.

Fix repository name not populated #496

Merged
merged 2 commits into from
Apr 2, 2020
Merged

Fix repository name not populated #496

merged 2 commits into from
Apr 2, 2020

Conversation

edaena
Copy link
Contributor

@edaena edaena commented Apr 2, 2020

Repository name is not part of the supported "opts"

@@ -143,7 +143,7 @@ export const fetchValidateValues = (
orgName: opts.orgName || "",
Copy link
Collaborator

@mtarng mtarng Apr 2, 2020

Choose a reason for hiding this comment

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

This whole logic section is kinda confusing. Why are we still loading from opts from here. Shouldn't all of these be loaded from the values object instead?

This was introduced in: https://github.com/CatalystCode/spk/pull/479/files

@edaena @dennisseah does this make sense for us to fix this all here? Does similar logic for other ConfigValues interfaces exist now too in other commands?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mtarng do you know other places that this problem too?

Copy link
Collaborator

@mtarng mtarng Apr 2, 2020

Choose a reason for hiding this comment

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

I'm not sure, which is why I asked the question @dennisseah . Have we refactored any other commands to move arguments from CommandOptions to ConfigValues?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will double check

@dennisseah dennisseah merged commit ebfa1f9 into master Apr 2, 2020
@samiyaakhtar samiyaakhtar deleted the edaena-reponame branch April 2, 2020 00:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants