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

fix: Default branch doesn't show environments #65

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

meniRoy
Copy link
Contributor

@meniRoy meniRoy commented Sep 19, 2023

problem:
Default branch doesn't show environments

Solution:
show env0 environments without blueprintRevision when current branch is the default branch
used git-simple to get the default branch
updated readme and errors accordingly

before:
b3

after:
a4

after when no envs to display
a3

Show environments without branch when the current branch is the default branch
@meniRoy
Copy link
Contributor Author

meniRoy commented Sep 19, 2023

Copy link

@Yossi-kerner Yossi-kerner left a comment

Choose a reason for hiding this comment

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

LGTM left some comments and would like to see some before and after screen shots

src/test/integration/suite/environments.test.it.ts Outdated Show resolved Hide resolved
src/get-environments.ts Outdated Show resolved Hide resolved
Comment on lines +49 to +55
const calls = showErrorMessageSpy.mock.calls.filter(
(call) =>
!(
call[0].startsWith(cannotGetDefaultBranchMessage) ||
call[0].startsWith(getDefaultBranchErrorMessage)
)
);

Choose a reason for hiding this comment

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

what errors? I don't understand this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the simple-git library is imported into the code before the test files load, making it impossible to mock, I am encountering error messages in the tests for getting the default branch. Therefore, I need to ignore the cannotGetDefaultBranchMessage and getDefaultBranchErrorMessage messages.

Choose a reason for hiding this comment

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

Wdym by its loaded before the test file? how is it different from other libs?

return undefined;
}

const remoteShowOrigin: string = await git.raw([

Choose a reason for hiding this comment

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

what does remoteShowOrigin mean?

I don't really understand how this functions works, can you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

I see, so the var name is a little misleading, can't it just be default branch? ( or maybe repo default branch )

src/utils/git.ts Show resolved Hide resolved
return {
repository: normalizedRepositoryName,
currentBranch,
isDefaultBranch,

Choose a reason for hiding this comment

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

Why is this part of this function? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why not 🤷🏻‍♂️

Choose a reason for hiding this comment

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

If its always used when this func is used then its fine imo ( but if its sometimes needed and sometimes it doesn't id remove it to its own func, because it doesn't really depend on anything here )

@meniRoy meniRoy requested a review from alonnoga September 26, 2023 23:55
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.

3 participants