search

rxdi / firelink

Firebase. gcloud and monorepos are not combining very well until they met @rxdi/firelink
MIT License
68 stars 9 forks source link

fix(createVirtualSymlink): pass success status to all calls to exitHandler #31

Closed jekozyra closed 2 years ago

jekozyra commented 2 years ago

Issue

Description

When running our deploys via CloudBuild using firelink 0.7.62, we found that it was exiting with a non-zero exit code sometimes even when the deploy command succeeded.

Upon investigation, it appears that there is sometimes a second call to exitHandler from process.on('exit', ... that occurs after the call to exitHandler following runCommand. Therefore, we need to ensure that the success status is passed to all calls to exitHandler. When testing this locally, it didn't occur consistently, so hopefully this mitigates the issue.

Type of change

Checklist:

Notes (specific implementation details)

I tested this by building firelink locally and creating several different situations in CloudBuild that I would expect to succeed or fail, and then validated that CI behaved as expected multiple times.

jekozyra commented 2 years ago

@Stradivario - how do I get the commit to show up properly in the changelog?

Stradivario commented 2 years ago

@Stradivario - how do I get the commit to show up properly in the changelog?

I am using conventional strategy for writing my commits with fix, feature, chore, etc. and i think you already did that here with the first commit https://github.com/rxdi/firelink/pull/31/commits/34b2e57d08c899a3e2bd6d550afca334d12944eb

Here you can read more about this strategy https://www.conventionalcommits.org/en/v1.0.0/

I decided to put Hook which will run on pre-commit using husky in order to collect the CHANGELOG.md.

The actual changelog is a bit buggy for the moment since it should write the commits for every tag that is created inside github. Now it just combines all of the commits to the latest release which is not nice. I think it broke at some point and i need to research this also. (https://github.com/rxdi/firelink/issues/33)

The PR looks really legit to me and i am really sorry that i didn't catch this on time!

I would like in some point to really refactor every line of code to be more maintainable using functional programming approach with Reader Monad 

export type PrivateReader<T, K> = (d: NonNullable<T>) => K;

It can be used this way 

const myFunction: Reader<[number], number> => () => ([value]) => value + 1;

myFunction()([1])

Or directly re-write the whole library using @rhtml/di this is the smallest Dependency injection that i have created at the moment. (https://github.com/rxdi/firelink/issues/34) https://github.com/r-html/rhtml/tree/master/packages/di

Also i need to remove the build step for binaries to be outside the repository with some github action since it makes the PR look dirty and with many changes. (https://github.com/rxdi/firelink/issues/32)

I have a telegram chat where we can freely discuss some bugs or issues you can join here if you want https://t.me/dependency

Cheers!