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

Issue-231 Fixed no Instances found #246

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

kitarp29
Copy link

Signed-off-by: kitarp29 <kitarpsinghrajpoot@gmail.com>
Copy link
Member

@alejandrojnm alejandrojnm left a comment

Choose a reason for hiding this comment

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

Hi @kitarp29 thanks for the PR, but more than this you can return a message before the loop like in the line

in that way we can avoid the execution of the rest of the code, it can be something like this

if len(instance) == 0 {
	utility.Info("We don't find any instance in the region %s", client.Region)
	os.Exit(1)
}

@kitarp29
Copy link
Author

kitarp29 commented Aug 20, 2022

Yes @alejandrojnm as I mentioned in my first message I knew that the rest of the code did not make sense to execute if len(instance)==0.
So should I commit the changes you mentioned or just add a break at the end of my code snippet?
cc: @kaihoffman

@RealHarshThakur
Copy link
Member

@kitarp29 thanks for the PR :)

Signed-off-by: kitarp29 <kitarpsinghrajpoot@gmail.com>
@alejandrojnm
Copy link
Member

We need to keep a standard in all the code, we can use println in some place and in others use utility.Info

@RealHarshThakur
Copy link
Member

Good point @alejandrojnm . We should move to a well-maintained logging package at some point like @kitarp29 pointed out in this issue. #233

Signed-off-by: kitarp29 <kitarpsinghrajpoot@gmail.com>
@RealHarshThakur
Copy link
Member

RealHarshThakur commented Aug 24, 2022

@alejandrojnm Do we want to hold this off until we resolve the logging issue?

Signed-off-by: kitarp29 <kitarpsinghrajpoot@gmail.com>
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.

3 participants