Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Invalid error after editing Dockerfile caused by incorrect handling of changes from textDocument/didChange #216

Closed
chrisdias opened this issue Mar 30, 2018 · 7 comments
Assignees
Labels

Comments

@chrisdias
Copy link

I'm testing the Docker extension for release, with 0.0.14 of the server.

Create this dockerfile:

FROM openjdk:8-jdk-alpine
VOLUME /tmp
ARG JAVA_OPTS
ENV JAVA_OPTS=$JAVA_OPTS
ADD stickerapp.jar stickerapp.jar
EXPOSE 3000
ENTRYPOINT exec java $JAVA_OPTS -jar stickerapp.jar
# For Spring-Boot project, use the entrypoint below to reduce Tomcat startup time.
#ENTRYPOINT exec java $JAVA_OPTS -Djava.security.egd=file:/dev/./urandom -jar stickerapp.jar

Put the cursor at the end of the line VOLUME /tmp and press backspace to delete /tmp
Press CMD+Z to undo your change so the line says VOLUME /tmp again.

Result: Red squiggles. Hover over ARG and see Invalid error [dockerfile-utils] Unknown instruction: /tmpARG

image

@rcjsuen
Copy link
Owner

rcjsuen commented Mar 31, 2018

Interesting. Thank you for the bug report, @chrisdias. I might not be able to look at it for a while pending hotel wifi...

@chrisdias
Copy link
Author

looks like this is in 0.0.13 as well.

@rcjsuen
Copy link
Owner

rcjsuen commented Mar 31, 2018

I couldn't reproduce this with the current version of the online editor. I guess that makes sense. It must be something to do with VS Code instead of Monaco.

@rcjsuen
Copy link
Owner

rcjsuen commented Apr 1, 2018

I was able to reproduce this problem. Interestingly enough, I received two events when I initiated the undo with the second being an empty array...

{
  "textDocument": {
    "uri": "file:///blah/blah/Dockerfile",
    "version": 9
  },
  "contentChanges": [
    {
      "range": {
        "start": {
          "line": 1,
          "character": 7
        },
        "end": {
          "line": 1,
          "character": 7
        }
      },
      "rangeLength": 0,
      "text": "/"
    },
    {
      "range": {
        "start": {
          "line": 1,
          "character": 8
        },
        "end": {
          "line": 1,
          "character": 8
        }
      },
      "rangeLength": 0,
      "text": "t"
    },
    {
      "range": {
        "start": {
          "line": 1,
          "character": 9
        },
        "end": {
          "line": 1,
          "character": 9
        }
      },
      "rangeLength": 0,
      "text": "m"
    },
    {
      "range": {
        "start": {
          "line": 1,
          "character": 10
        },
        "end": {
          "line": 1,
          "character": 10
        }
      },
      "rangeLength": 0,
      "text": "p"
    }
  ]
}
{
  "textDocument": {
    "uri": "file:///blah/blah/Dockerfile",
    "version": 9
  },
  "contentChanges": []
}

@rcjsuen
Copy link
Owner

rcjsuen commented Apr 1, 2018

This is caused by the fix for #58. It seems like we'll need to figure out how to merge and sort the received contentChanges better.

@rcjsuen
Copy link
Owner

rcjsuen commented Apr 1, 2018

This is caused by the fix for #58. It seems like we'll need to figure out how to merge and sort the received contentChanges better.

Actually, I think I've just been misinterpreting the specification this whole time. I've always applied the changes in reverse order but I don't remember why. However, given that the specification states:

	/**
	 * The actual content changes. The content changes describe single state changes
	 * to the document. So if there are two content changes c1 and c2 for a document
	 * in state S10 then c1 move the document to S11 and c2 to S12.
	 */
	contentChanges: TextDocumentContentChangeEvent[];

It does sound like the changes should just be applied in the order it appears in the notification.

@rcjsuen rcjsuen closed this as completed in cacb1a2 Apr 2, 2018
@rcjsuen rcjsuen added the bug label Apr 2, 2018
@rcjsuen rcjsuen self-assigned this Apr 2, 2018
@rcjsuen rcjsuen changed the title invalid error after editing dockerfile Invalid error after editing Dockerfile caused by incorrect handling of changes from textDocument/didChange Apr 2, 2018
rcjsuen added a commit that referenced this issue Apr 2, 2018
Remove a function that is no longer used now that we are no longer
trying to sort the changes received from textDocument/didChange.

Signed-off-by: Remy Suen <[email protected]>
@rcjsuen
Copy link
Owner

rcjsuen commented Apr 2, 2018

@chrisdias Thanks again for the bug report! I've fixed the bug now and it will be included in the next release!

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

No branches or pull requests

2 participants