Skip to content

chore: improve debugging for no subnets error in task run#2023

Closed
ota42y wants to merge 4 commits intoaws:mainlinefrom
ota42y:feature/add_subnets_error_message
Closed

chore: improve debugging for no subnets error in task run#2023
ota42y wants to merge 4 commits intoaws:mainlinefrom
ota42y:feature/add_subnets_error_message

Conversation

@ota42y
Copy link
Copy Markdown
Contributor

@ota42y ota42y commented Mar 6, 2021

Improve error message to let users know that a tag is required for subnet to run task.

Please read this issue
#2022

@ota42y ota42y requested a review from a team as a code owner March 6, 2021 03:03
@ota42y ota42y requested a review from efekarakus March 6, 2021 03:03
@ota42y ota42y changed the title add subnets error messages Bug Fixes: add subnets error messages Mar 6, 2021
@ota42y ota42y changed the title Bug Fixes: add subnets error messages fix: add subnets error messages Mar 6, 2021
@ota42y ota42y force-pushed the feature/add_subnets_error_message branch from b162291 to ee77888 Compare March 6, 2021 03:14
Copy link
Copy Markdown
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

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

Awesome thank you so much for making debugging this issue better 🙇 ! Some small suggestions below

Comment thread internal/pkg/task/env_runner.go Outdated
Comment thread internal/pkg/task/env_runner.go Outdated
@efekarakus efekarakus changed the title fix: add subnets error messages chore: improve debugging for no subnets error in task run Mar 8, 2021
@ota42y
Copy link
Copy Markdown
Contributor Author

ota42y commented Mar 9, 2021

I have completed the fix!
I changed to show a warning by error type in cli package so I made error struct public for error check.

@efekarakus efekarakus added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Mar 9, 2021
Copy link
Copy Markdown
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

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

Yay this looks great! Thank you so much, I left some small nits. Once addressed, we'll remove the "do-not-merge" label which will make @mergifyio automatically merge your PR 🚀

Comment thread internal/pkg/cli/task_run.go Outdated
Comment on lines 11 to +12
var (
errNoSubnetFound = errors.New("no subnets found")
ErrNoSubnetFound = errors.New("no subnets found")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we add a top-level comment here since we now have exported errors

Suggested change
var (
errNoSubnetFound = errors.New("no subnets found")
ErrNoSubnetFound = errors.New("no subnets found")
// Errors returned while trying to run a task.
var (
ErrNoSubnetFound = errors.New("no subnets found")

ota42y and others added 2 commits March 10, 2021 13:12
Co-authored-by: Efe Karakus <efekarakus@gmail.com>
@ota42y
Copy link
Copy Markdown
Contributor Author

ota42y commented Mar 10, 2021

I fixed it.

However, if you merge #2024 to next version, you must not merge this PR.
Because I changed to use Stack instead of tags in #2024 , so this warning message is no longer needed.
If you don't merge #2024 to next version, this message is useful for users.

@efekarakus
Copy link
Copy Markdown
Contributor

Yay sounds good! We'll merge #2024 instead!

@ota42y
Copy link
Copy Markdown
Contributor Author

ota42y commented Mar 10, 2021

Ok, I'm going to close this PR when #2024 merged

@ota42y
Copy link
Copy Markdown
Contributor Author

ota42y commented Mar 11, 2021

#2024 is merged 🎉

@ota42y ota42y closed this Mar 11, 2021
@ota42y ota42y deleted the feature/add_subnets_error_message branch March 11, 2021 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge Pull requests that mergify shouldn't merge until the requester allows it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants