Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [che-dev] DTO behavior change

I have just updated the PR description with an environment variable, that allows enabling the old way of serialization. I am right now doing last checks for it, and then will notify Vitaliy that it is ready for release.

On Tue, Jun 18, 2019 at 10:52 AM Mario <mario.loriedo@xxxxxxxxx> wrote:
Sergii can we have an update here? 

It looks like we are still waiting for PR [1] to get merged and the release of 7.0.0-RC2 is blocked because of it. The solution is to use a configuration property as suggested by Florent. The fix should be available today and the release process should start right after the PR get merged. Sergii is that correct?



On Fri, Jun 14, 2019 at 3:20 PM Sergii Kabashniuk <skabashn@xxxxxxxxxx> wrote:


On Fri, Jun 14, 2019 at 3:47 PM Florent Benoit <florent@xxxxxxxxxx> wrote:
@sergii yes sure but you said this kind of configuration switch was quite impossible to do quickly.
 
Making proper configuration injection is hard, doing environment variable check is a quick and dirty hack to provide the ability  
to have an old style of DTO serialization.


It's good to ensure people have workarounds if they're facing problems as for now as it was only tested in a branch and never went live and often people complain only when they encounter the issue, not by reading eagerly all planned issues.

Florent



On Fri, Jun 14, 2019 at 2:40 PM Sergii Kabashniuk <skabashn@xxxxxxxxxx> wrote:
@Brad Micklea discussion on this topic is already going ~ 2 days [1]
We faced it during implementing [2] and it can potentially hold us in [3] and [4]

At this moment CR2 on hold because of this PR[1]
@Florent Benoit  would it be ok for you to uphold this PR if we provide a temporary environment variable that will force the old behavior of DTO serialization?

[1] Do not serialize DTO fields with empty or null objects and arrays  https://github.com/eclipse/che/pull/13515#pullrequestreview-248681856
[2] Deprecate storing of workspaces converted from Devfile https://github.com/eclipse/che/issues/13423
[3] Devfile not validated on workspace update https://github.com/eclipse/che/issues/13526
[4] Add the ability to validate json during DtoFactory deserialization https://github.com/eclipse/che/issues/13527

On Fri, Jun 14, 2019 at 3:07 PM Brad Micklea <bmicklea@xxxxxxxxxx> wrote:
Please hold on merge - the timeframe for this discussion was overnight for anyone based in North America. We need to give people the opportunity to consider the impact of this change. If getting this merged today was a priority we should have started the discussion several days ago.

Can someone add the issue link here as well please?

On Fri, Jun 14, 2019 at 08:03 Florent Benoit <florent@xxxxxxxxxx> wrote:
ok so I guess there was no discussion at all ?

Florent

On Fri, Jun 14, 2019 at 1:42 PM Sergii Kabashniuk <skabashn@xxxxxxxxxx> wrote:
QA passed. We are going to merge in 20 minutes unless something critical happened.

On Fri, Jun 14, 2019 at 11:52 AM Sergii Kabashniuk <skabashn@xxxxxxxxxx> wrote:
Ideally, we would like to have this change in CR2 (today) and left some space to test it before GA.
An alternative to it is not to have devfile validation on workspace creation/editing.
That is the worst case scenario IMO.

On Fri, Jun 14, 2019 at 11:25 AM Sergii Kabashniuk <skabashn@xxxxxxxxxx> wrote:


On Fri, Jun 14, 2019 at 11:11 AM <tmader@xxxxxxxxxx> wrote:
Hi Sergii,

On Fri, 2019-06-14 at 09:13 +0300, Sergii Kabashniuk wrote:
Hello

I would like to share a note about the change in DTO behavior on Che master side.

Previously in during DTO serialization, we put everything to the output including
empty collections/fields. At this moment we are not able to identify the concrete reason why we did this way and not the opposite.

In typescript, for example, mandatory but possibly empty and optional are two different types. This may make it harder for some clients to work with the API. Apart from the null checks necessary, I believe we are using some JSON schemas in the API (e.g. kubernetes) where we don't control the definition.


This behavior brings to us some difficulties when we tried to add devfile schema validation in our REST API.
So we decided to rethink this behavior again.  As a result, we think that it makes sense to revert this behavior and do not serialize empty collections/fields. That will allow reducing the size of result json. As a side effect all clients (most of them already doing that)
has to check optional field != null before doing any operation with it.

As we are working towards Che 7 GA, we should restrict our changes progressively to those that are strictly necessary for that goal. Could you explain a bit what the problem is with devfile validation and how this change helps? I think that would improve buy-in from clients.

Let's take two components as an example.

{
  "components": [
    {
      "alias": "mysql",
      "type": "kubernetes",
      "referenceContent": "petclinic.yaml",
      "selector": {
        "app.kubernetes.io/part-of": "petclinic",
        "app.kubernetes.io/component": "database",
        "app.kubernetes.io/name": "mysql"
      }
    }
  ]
}


{
  "components": [
    {
      "alias": "theia",
      "type": "cheEditor",
      "id": "eclipse/chetheia/0.0.3"
    }
  ]
}

In our schema, we defined that only kubernetes component can contain selector. However our DTO mechanism
will serialize cheEditor with empty selector 
{
  "components": [
    {
      "alias": "theia", 
      "type": "cheEditor", 
      "id": "eclipse/chetheia/0.0.3", 
      "selector": null
    }
  ]
}
and schema validator will complain about wrong filed "selector".

/Thomas

_______________________________________________
che-dev mailing list
che-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/che-dev


--

Sergii Kabashniuk

Principal Software Engineer, DevTools 

Red Hat Ukraine

skabashniuk@xxxxxxxxxx    



--

Sergii Kabashniuk

Principal Software Engineer, DevTools 

Red Hat Ukraine

skabashniuk@xxxxxxxxxx    



--

Sergii Kabashniuk

Principal Software Engineer, DevTools 

Red Hat Ukraine

skabashniuk@xxxxxxxxxx    

_______________________________________________
che-dev mailing list
che-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/che-dev
_______________________________________________
che-dev mailing list
che-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/che-dev
--
Brad Micklea // Group Lead, Developer Tools & Program // 416.707.0792 
_______________________________________________
che-dev mailing list
che-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/che-dev


--

Sergii Kabashniuk

Principal Software Engineer, DevTools 

Red Hat Ukraine

skabashniuk@xxxxxxxxxx    



--

Sergii Kabashniuk

Principal Software Engineer, DevTools 

Red Hat Ukraine

skabashniuk@xxxxxxxxxx    

_______________________________________________
che-dev mailing list
che-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/che-dev
_______________________________________________
che-dev mailing list
che-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/che-dev


--

MYKHAILO KUZNIETSOV

Red Hat 

mkuznets@xxxxxxxxxx   


Back to the top