Skip to content

[Concourse] Navigation in pipeline files with VSCode Concourse CI extension doesn't work everytime #483

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
OphyTe opened this issue Jun 17, 2020 · 21 comments
Milestone

Comments

@OphyTe
Copy link

OphyTe commented Jun 17, 2020

Sometimes, navigation through the outline view doesn't work anymore on the pipeline files ... I get the message "No symbols found in document pipeline.yaml' (my document is ok, it works on my Concourse instance).

I didn't know why yet. Maybe because of my use of yaml anchors and aliases ...

@kdvolder
Copy link
Member

Does the problem seem deterministic? I.e. is it something that depends on the contents in the editor and allways happens when that editor content is the same? Or is more like something that feels a bit random, it works / doesn't work even though nothing is different about the editor content.

If it is deterministic, it would be very helpful to provide the exact editor content that causes the problem (or a cut down version of it). That should make it relatively easy then to reproduce, diagnose and fix.

@kdvolder
Copy link
Member

kdvolder commented Jun 17, 2020

Followed up on your hunch that it might have something to do with anchors and references. And... bingo :-)

This example reproduces the issue:

def: &stuff
  name: git
  type: git
  source:
    uri: 
resources:
- <<: *stuff
- name: git2
  type: git
  source:
    uri: git@github.com:kdvolder/7zip.git

When I delete the line - <<: *stuff the outline appears. Put the line back... it disapears again.

@OphyTe
Copy link
Author

OphyTe commented Jun 18, 2020

Thank you for you answer but it doesn't seem to be so simple.

Indeed, when I delete the lines with anchors or aliases, my outline view doesn't work better.
And on another project I do have a well working outline view for a pipeline.yaml with 1 anchor and 14 aliases ...

Here's a sample:

registries-pe: &registries-pe
  insecure_registries:
    - ((docker_pe.registre))
  username: ((docker_pe.utilisateur))
  password: ((docker_pe.mot_de_passe))
  registry_mirror: http://((docker_pe.registre))

resource_types:

  - name: kubernetes-resource
    type: docker-image
    source:
      repository: ((docker_pe.registre))/incubateur/concourse-kubernetes-resource
      tag: 1.16
      <<: *registries-pe

  - name: concourse-pipeline-resource
    type: docker-image
    source:
      repository: ((docker_pe.registre))/concourse/concourse-pipeline-resource
      tag: 1.1.0
      insecure_registries:
        - ((docker_pe.registre))
      <<: *registries-pe

@kdvolder
Copy link
Member

Thanks for the extra examples. It is always nice to have more examples to demonstrate / reproduce a bug, as well as verify a potential fix. This is especially true if the examples are markedly different. So therefore... you said:

when I delete the lines with anchors or aliases, my outline view doesn't work better

So I take it you actually have a editor contents example that has a problem despite not using anchors and/or references? Can you please paste that example as it would be a bit different from the other examples we have already.

@OphyTe
Copy link
Author

OphyTe commented Jun 18, 2020

Here is an example (I rename it in .txt so I can upload it on github): pipeline.yaml

And here is the corresponding outline view and the non-working GoTo functionality:
image

@OphyTe
Copy link
Author

OphyTe commented Jun 18, 2020

I've just noticed that if if I disable the plugin, reload VSC and then finally reactivate the plugin, I get the outline back again (with the file without anchors and/or references) ...

The same process doesn't work with the file with anchors and/or references.

@kdvolder
Copy link
Member

Looks to me like contents of the outline view is simply not being updated immediately in all cases.
I think this part of the logic is most likely due to vscode itself rather than our extension.

I'm not sure of the exact logic of when vscode refreshes the outline. Saving the file seems to be fairly robust way of 'forcing' an update. Sometimes it also refreshed quickly as I typed, sometimes there seems to be a lag. And sometimes, unless I do something l(like click in the editor or save it) it doesn't refresh.

However, from what I can tell so far, the problem with the outine being 'empty' is tied strictly to references and anchors. A file with no references / anchors in it seems to allways work fine... sometimes we have to do something to make it appear (save file etc).

And yes, you are right some examples with references in them do still seem to work fine. That's interesting too. But most important is... having some examples that makes it not work, with those I can debug the issue and figure out why it doesn't work... then hopefully fix that.

BTW: Ironically, all the examples that I've tried so far, only the first one that I posted myself... is one were it breaks outine for me. The other two you gave both seem to work just fine.

@OphyTe
Copy link
Author

OphyTe commented Sep 25, 2020

Hi @kdvolder,

I come back on this issue and after a lot of tests, I'm starting to think that the issue comes from the number of references and anchors used.

Indeed, on this file it works but on this one it doesn't!

@kdvolder
Copy link
Member

Thanks for the extra example. That helps.

@kdvolder
Copy link
Member

Problems that I found debugging this:

  • some symbols where computed as 'null' because the way we tried to find their name didn't properly traverse trhough a '<<' reference
  • symbols like in my example end up with their name range being outside of the symbols range (because the reference points to somewhere 'outside' its range and that is where the namerange is.

Vscode doesn't like 'null' in the tree neither does it like symbol's who's name range isn't 'nested' in he symbol range.

I addressed both of these by:

  • teaching the 'yaml travers' helper functions to handle '<<' references
  • adding an extra null check to avoid null symbols should it somehow still happen
  • detecting when a 'name is outside of symbol' situation arises and changing the symbol range to avoid that situation.

Now my own example seems to work. It would nice @OphyTe if you could also give it a try and see if it helps for your examples as well.

@kdvolder
Copy link
Member

If you want to try, you can get a snasphot build from here: http://dist.springsource.com/snapshot/STS4/nightly-distributions.html

@kdvolder
Copy link
Member

I decided to quickly try out your complex 'not working' example from above. Sadly that still is not working.

It's quite a complex file that one :-). So not so easy to just eyeball it and guess why it's causing issues. It's getting a bit late here and I'm done for today, but before closing this ticket, I'd like to try debug this a bit and understand what may be going on there.

So reopening ticket until I have a chance to do that.

The snasphot meanwhile already has the fixes that may help in some cases. Still would be nice if you can give that one a try.

@kdvolder kdvolder reopened this Nov 14, 2020
@OphyTe
Copy link
Author

OphyTe commented Nov 15, 2020

I've just tested it and it doesn't work on any of my projects ... 😞

@kdvolder
Copy link
Member

I created a test case from the 'not working' sample. Looks like when our editor is trying to validate the file, the validation is crashing with some error:

java.lang.IllegalArgumentException: Multiple entries with same key: <org.yaml.snakeyaml.nodes.ScalarNode (tag=tag:yaml.org,2002:str, value=mattermost-notify)>=Resource Name and <org.yaml.snakeyaml.nodes.ScalarNode (tag=tag:yaml.org,2002:str, value=mattermost-notify)>=Resource Name
	at com.google.common.collect.ImmutableMap.checkNoConflict(ImmutableMap.java:136)
	at com.google.common.collect.RegularImmutableMap.checkNoConflictInKeyBucket(RegularImmutableMap.java:98)
	at com.google.common.collect.RegularImmutableMap.fromEntryArray(RegularImmutableMap.java:84)
	at com.google.common.collect.ImmutableMap$Builder.build(ImmutableMap.java:295)
	at org.springframework.ide.vscode.commons.yaml.reconcile.ASTTypeCache.endCollecting(ASTTypeCache.java:107)
	at org.springframework.ide.vscode.commons.yaml.reconcile.YamlSchemaBasedReconcileEngine$1.endCollecting(YamlSchemaBasedReconcileEngine.java:63)
	at org.springframework.ide.vscode.commons.yaml.reconcile.SchemaBasedYamlASTReconciler.reconcile(SchemaBasedYamlASTReconciler.java:122)
	at org.springframework.ide.vscode.commons.yaml.reconcile.YamlReconcileEngine.reconcile(YamlReconcileEngine.java:45)
	at org.springframework.ide.vscode.commons.languageserver.util.SimpleLanguageServer.lambda$validateWith$11(SimpleLanguageServer.java:626)
	at reactor.core.publisher.MonoRunnable.subscribe(MonoRunnable.java:49)
	at reactor.core.publisher.Mono.subscribe(Mono.java:4210)
	at reactor.core.publisher.MonoSubscribeOn$SubscribeOnSubscriber.run(MonoSubscribeOn.java:124)
	at reactor.core.scheduler.WorkerTask.call(WorkerTask.java:84)
	at reactor.core.scheduler.WorkerTask.call(WorkerTask.java:37)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
	at java.base/java.lang.Thread.run(Thread.java:832)

I'm looking into that now trying to determine the cause of it. First guess: the use of anchors and references makes it so that the yaml AST is not strictly a tree anymore. There are sections of the 'tree' that are actually shared and so when we 'walk' the tree we are visiting some parts of the tree more than once. I think this causing problems somehow.

@kdvolder
Copy link
Member

Note: whatever the cause may be, this is clearly to be considered as a bug in our validator. Even if there is 'strange stuff' or problems in a file (and I'm not even sure there is an actual problem with the file)... the validator should never crash, (if there is a problem, a problem should be reported, instead of validator crashing).

@kdvolder
Copy link
Member

Looks like I was correct in my guess. Added some logging to detect the condition that would cause the exception and then log some info instead of crashing.

I got a very long list of this:

14:08:47.528 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - Ignore assignment of type for: YamlPath([0], .jobs, [0], .plan, [100], .on_failure, .resource)
14:08:47.528 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache -           Already assigned at: YamlPath([0], .jobs, [0], .plan, [1], .on_failure, .resource)
14:08:47.528 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - Previous type = Resource Name
14:08:47.528 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - New      type = SAME
14:08:47.528 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - Ignore assignment of type for: YamlPath([0], .jobs, [0], .plan, [101], .on_failure, .resource)
14:08:47.528 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache -           Already assigned at: YamlPath([0], .jobs, [0], .plan, [1], .on_failure, .resource)
14:08:47.528 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - Previous type = Resource Name
14:08:47.528 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - New      type = SAME
14:08:47.529 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - Ignore assignment of type for: YamlPath([0], .jobs, [0], .plan, [102], .on_failure, .resource)
14:08:47.529 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache -           Already assigned at: YamlPath([0], .jobs, [0], .plan, [1], .on_failure, .resource)
14:08:47.529 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - Previous type = Resource Name
14:08:47.529 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - New      type = SAME
14:08:47.529 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - Ignore assignment of type for: YamlPath([0], .jobs, [0], .plan, [103], .on_failure, .resource)
14:08:47.529 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache -           Already assigned at: YamlPath([0], .jobs, [0], .plan, [1], .on_failure, .resource)
14:08:47.529 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - Previous type = Resource Name
14:08:47.529 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - New      type = SAME
14:08:47.530 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - Ignore assignment of type for: YamlPath([0], .jobs, [0], .plan, [104], .on_failure, .resource)
14:08:47.530 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache -           Already assigned at: YamlPath([0], .jobs, [0], .plan, [1], .on_failure, .resource)
14:08:47.530 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - Previous type = Resource Name
14:08:47.530 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - New      type = SAME
14:08:47.530 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - Ignore assignment of type for: YamlPath([0], .jobs, [0], .plan, [105], .on_failure, .resource)
14:08:47.530 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache -           Already assigned at: YamlPath([0], .jobs, [0], .plan, [1], .on_failure, .resource)
14:08:47.530 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - Previous type = Resource Name
14:08:47.530 [Reconciler-1] INFO  o.s.i.v.c.y.reconcile.ASTTypeCache - New      type = SAME
...

@kdvolder
Copy link
Member

In the sample file the 'notifications' section with a 'on_failure' bit that has a reference to a resource in it.

This is inserted by means of <<: *notifications into a lot of tasks. And it this piece that is visited by the validation many times. Each time we see the 'resource reference' we try to enter the type of the node into a map from node->type. It as assumed that we'd only visit each node once. But now... here that isn't true, so we try to assign the same node again. guava immutable map builder detects that as a problem and throws an exception.

@kdvolder
Copy link
Member

Good news for this example is that it seems the type assignment in each visit is consistent. This makes sense, if the inserted blob of yaml is used in each 'insertion point' in a similar way. So, if we just detect the duplicate type assignment and simply ignore the extra/redundant ones (instead of allowing them to crash the validator), I think everything should work nicely.

kdvolder added a commit that referenced this issue Nov 16, 2020
... and fix problem it triggers.
@kdvolder
Copy link
Member

The newest snapshot now works with your previously posted 'complex not working example'.

Snapshot that I tried out myself: https://s3-us-west-1.amazonaws.com/s3-test.spring.io/sts4/vscode-extensions/snapshots/vscode-concourse-1.24.0-202011162342.vsix

Can be gotten also from http://dist.springsource.com/snapshot/STS4/nightly-distributions.html (but be careful as that browser has a tendency to cache old links / contents of that page. So make sure version you get is at least timestamp 202011162342.

Let me know how it goes. If you still have issue... as always... if you can provide an example of something that doesn't work would be great to help debug.

@martinlippert martinlippert removed this from the 4.8.1.RELEASE milestone Nov 17, 2020
@martinlippert martinlippert added this to the 4.9.0.RELEASE milestone Nov 17, 2020
@OphyTe
Copy link
Author

OphyTe commented Nov 17, 2020

Yes it works like a charm!
And it seems to be faster!
Perfect! Thanks a lot 👍

All that's missing now is the integration of the anchors in the outline view 😉

@kdvolder
Copy link
Member

Great! Thanks for trying it.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants