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

Refactor the command for creating a local development environment #39

Merged

Conversation

TomerFi
Copy link
Collaborator

@TomerFi TomerFi commented Apr 19, 2022

No description provided.

Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>
Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>
Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>
@TomerFi TomerFi requested a review from ilan-pinto April 19, 2022 22:07
Copy link
Owner

@ilan-pinto ilan-pinto left a comment

Choose a reason for hiding this comment

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

good points

  • i really liked the breakdown you made with the build.ts file
  • i really liked the input process

in discussion point:

  • using .then instead of await function buildLocalEnv created a very messy code that is hard to track or understand. IMO we should use async/await

  • executeShellCommand should support sync and async.

progressReporter({increment: 100 , message: 'failed to initialize the hub cluster'});
reject('OCM extension, failed to initialize the hub cluster');
});
})
Copy link
Owner

Choose a reason for hiding this comment

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

this is less clean than using await.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think so.
before, after each await statement, we needed to sleep,
using this callbacks technique, and we sleep no more.
:-)

Copy link
Collaborator Author

@TomerFi TomerFi Apr 20, 2022

Choose a reason for hiding this comment

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

also, with the awaits statements,
we dropped granular control of the displayed message.

we didn't handle rejected promises,
we just let them all drop into the catch clause of the try-catch statement.

now, using the catch callbacks, we handle rejections better at the time they arrive,
we have full control over what is being displayed to the user, and what is being done after the rejection.

if we were to try and do this with previous iteration using the await statements,
I'm guessing it wouldn't have looked all that well with a million try-catch/switch/if statements.
🤔

IMO
the use of the await statements was cool and pretty,
but, as it was, it didn't get half the work done as the current promise and callbacks technique,
so I'm not sure the two code iterations are comparable.
😃

Copy link
Owner

Choose a reason for hiding this comment

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

but we could handle the rejection of the promise.
e.g
acceptJoinRequests rejection promise could also take care of

console.debug('failed to accept join requests');
console.error(stderr);
progressReporter({increment: 100 , message: 'failed to accept the join request made from the managed clusters'});
reject('OCM extension, failed to accept the join request made from the managed clusters');

src/utils/build.ts Show resolved Hide resolved
Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>
Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>
@TomerFi TomerFi requested a review from ilan-pinto April 20, 2022 06:57
@TomerFi
Copy link
Collaborator Author

TomerFi commented Apr 20, 2022

@ilan-pinto

good points

  • i really liked the breakdown you made with the build.ts file
  • i really liked the input process

Thanks. :-)

in discussion point:

  • using .then instead of await function buildLocalEnv created a very messy code that is hard to track or understand. IMO we should use async/await

I think it looks better this way, about the readability,
perhaps we can break it down into smaller functions and make it more readable,
or maybe emphasize the code documentation to make it clearer.

  • executeShellCommand should support sync and async.

I should have pointed out, I fixed executeShellCommand.
you were correct, in its previous iteration it was blocking (my bug).
you added an async boolean argument and made it do both sync and async.
but now... it was modified, it's async for all.
:-)

Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>
Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>
Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>
let kindClustersCreated = Promise.all(clusterPromises);
kindClustersCreated
.then(() => {
/* ################################ ##
Copy link
Owner

Choose a reason for hiding this comment

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

doesn't need to be so big
// Initialize the Hub Cluster

could be enough

Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>
@TomerFi TomerFi requested a review from ilan-pinto April 20, 2022 20:11
Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>
Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>
Copy link
Owner

@ilan-pinto ilan-pinto left a comment

Choose a reason for hiding this comment

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

very good!
you can merge it to my branch

@ilan-pinto ilan-pinto merged commit 6bcc43d into ilan-pinto:3-kind-dev-environment Apr 21, 2022
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.

2 participants