thetanz / OpenFeature-al

OpenFeature for AL transforms Application Areas into feature toggles.
https://theta.co.nz
GNU Lesser General Public License v3.0
21 stars 5 forks source link

Extension somehow breaks other extensions install #18

Closed StefanMaron closed 1 year ago

StefanMaron commented 1 year ago

The Error message when I try to install my extension

{"error":{"code":"Unknown","message":"The specified file is not an extension file. (Parameter 'stream') CorrelationId: 6851a81b-c3c3-4986-9978-d8a953dbda9b."}} I was able to reproduce this with postman.

When ever I unstall the open feature app in "clean" mode, so with removing all data that belongs to it. It will get pulled and installed from appsource automatically and my publish will work again. Otherwise I get the error stated above.

Oddly, this only happens if deployed though my pipeline. Through the UI and DEV endpoint it works without problems. And even if I remove the dependency, the error persists. Only the complete removal of the Open Feature extension will solve this.

Let me know if there is any think I can give, that could help you.

StefanMaron commented 1 year ago

The fix I thought would help was wrong.

After some more debugging it seems like a session that deploys code (maybe all S2S sessions?) do not have application areas: image

image

StefanMaron commented 1 year ago

@vody I am not exactly sure what the fix could look like. I would probably just skip the entire function if ApplicationArea() returns '', but I am not sure if this would not break something else

vody commented 1 year ago

@StefanMaron, yep. I thought this as well. Spend a day trying to understand why it will be blank to identify if it will be ok to skip setting other flags. It looks like it's something to do with a system user {00000...001}. This deployment are executed from this user id.

StefanMaron commented 1 year ago

Did you find any other way to identify those kind of sessions? Since API and Background would still include regular sessions

Maybe also just tie it to the system user? That would be the most pessimistic solution, leaving possible other scenarios still broken

vody commented 1 year ago

@StefanMaron, we did see this in early versions where you have to have it blank or have all valid values. If it's blank then the system skips this ApplicationArea logic completely. But if even something is defined like "#FFTSL" then it's trying to enable it and looks like getting broken because "#All" is not there. The current fix should resolve it and I will get more investigation further to get it in more detail.

vody commented 1 year ago

@StefanMaron, will push a new release to AppSource later today. Still manually as need to plug an ALGo into this repo to get it automatic. :)

StefanMaron commented 1 year ago

@vody Please have a look at my Screenshot again. You need to use ApplicationArea() instead of GetApplicationAreaSetup() to do the check

vody commented 1 year ago

@StefanMaron , sure. Will review it.

vody commented 1 year ago

@StefanMaron, this fix is more complete. Because ApplicationArea will be blank for background sessions, we need to use an internally cached state of features to respond on IsEnabled API. We can skip setting ApplicationArea for this case as no visual controls are involved. The last point for me is to check how this will affect API pages where ApplicaitonArea may define which fields are available for API and which are not.

We had an issue with signing the certificate, but this should be resolved on Monday when the new release goes out.

StefanMaron commented 1 year ago

Just to understand: The isEnabled will work without analyzing the application areas so conditions in code can still use it in background sessions?

vody commented 1 year ago

@StefanMaron, yes. That is correct.

StefanMaron commented 1 year ago

@vody Any ETA on the update for AppSource?

vody commented 1 year ago

@StefanMaron, was hoping to get it out today, but didn't have a certificate sorted yet.Will hope to get it tomorrow as the latest one.

vody commented 1 year ago

@StefanMaron, we had an issue with GlobalSign—our signing certificate provider. I don't have any ETA on when it will be resolved, but I hope to get it this week.

vody commented 1 year ago

It is fixed in v3.3.0.0. @StefanMaron, please confirm so we can close this issue.

vody commented 1 year ago

@StefanMaron, I verify with our internal app with AL-Go automation. Closing as resolved. Please comment if there will be any issues on your side.