search

kiegroup / github-action-build-chain

85 stars 24 forks source link

fix: validate workspace against state file before resuming #453

Closed shubhbapna closed 1 year ago

shubhbapna commented 1 year ago

fixes #452

I think the only thing that needs to be verified is the state of the repositories in the workspace. Verifying whether the actual state file is valid or not might defeat the purpose of the resume command. So I think we have to operate with the assumption that the user will not make changes to the state file itself.

shubhbapna commented 1 year ago

If I do expose an option to change the definition file then we have to run the build process from the start so it isn't resume anymore. It will be equivalent to running one of the branch, single, pr or fd build. So I am not sure if we should expose changing the definition file. Wdyt?

lampajr commented 1 year ago

If I do expose an option to change the definition file then we have to run the build process from the start so it isn't resume anymore. It will be equivalent to running one of the branch, single, pr or fd build. So I am not sure if we should expose changing the definition file. Wdyt?

well yes, but consider the case where we have many projects to be built and you added a typo in the last build script for the last project - right now you would have to re-build everything which could take many hours - and resuming would become useless as the build script would be still broken

lampajr commented 1 year ago

my idea was more to build starting from the project that failed, taking the new changes provided by command line - i.e., everything already built won't be rebuilt

shubhbapna commented 1 year ago

ok I see that make sense.

But what if the user passed a completely new definition file. All the previous calculations done such as generating checkout info will be useless and it will have to generate all of it again. Moreover, for build-chain it won't be able to differentiate between a "bad" definition file (the one mentioned here) and a "good" definition file (the one you mentioned) and so will have to rebuild anyways. Should we again assume the user is responsible for a giving a "good" definition file? Or is there a better way?

lampajr commented 1 year ago

yeah I see your point and it is not that easy you are right, on the other hand we are not forced to let user override the whole definition file - maybe we can just expose a way to override build-script for a specific project that is not built yet - I mean we have full control on what we can expose and what not. I am just thinking loud here.. maybe we can move the discussion out of this pr as I think it would be more an enhancement FMPOV, wdyt?

shubhbapna commented 1 year ago

@Ginxo I wasn't sure. Should I upgrade the version once I have finished all the other smaller tasks related to resume or should i keep bumping patch right now?

shubhbapna commented 1 year ago

yeah I see your point and it is not that easy you are right, on the other hand we are not forced to let user override the whole definition file - maybe we can just expose a way to override build-script for a specific project that is not built yet - I mean we have full control on what we can expose and what not. I am just thinking loud here.. maybe we can move the discussion out of this pr as I think it would be more an enhancement FMPOV, wdyt?

@lampajr sounds good, i will create a separate issue for it

Ginxo commented 1 year ago

@Ginxo I wasn't sure. Should I upgrade the version once I have finished all the other smaller tasks related to resume or should i keep bumping patch right now?

@shubhbapna any feature or improvement should be provided to the user once it is finished, and this is the case FMPOV. Don't hesitate to publish whatever the amount of features or new versions it is required (specially on JS world) wdyt?

shubhbapna commented 1 year ago

Sounds good!