Skip to content

update resource version to take a map string->string#24

Merged
kdvolder merged 1 commit into
spring-projects:masterfrom
srbry:master
Dec 12, 2017
Merged

update resource version to take a map string->string#24
kdvolder merged 1 commit into
spring-projects:masterfrom
srbry:master

Conversation

@srbry

@srbry srbry commented Dec 8, 2017

Copy link
Copy Markdown
Contributor

Versions tags on resources should be a map not a "version" as defined elsewhere within a pipeline.

Reference:

version: object

Optional. A specific version of the resource to fetch. This should be a map with string keys and values. If not specified, the latest version will be fetched.

Example:

    - get: cf-deployment-git
      version: { ref: ((cf_deployment_commit_ref)) }

Signed-off-by: Ashish Sehra <asehra@gmail.com>
@martinlippert

Copy link
Copy Markdown
Member

@kdvolder can you have a second look at this? Looks fine and good for merging from my side.

@kdvolder

Copy link
Copy Markdown
Member

Looks good. I can accept this PR and I'll probably add a test case too.

@kdvolder kdvolder merged commit 8c771e2 into spring-projects:master Dec 12, 2017
kdvolder added a commit that referenced this pull request Dec 12, 2017
@kdvolder

Copy link
Copy Markdown
Member

The fix is available in snapshot vscode extension now. From here: http://dist.springsource.com/snapshot/STS4/nightly-distributions.html

@kdvolder

Copy link
Copy Markdown
Member

Quick follow up question for @srbry ... in case you know the anser :-)

I accepted the PR as is and just added a simple test case. However upon reading the docs, I'm not entirely sure whether declaring this as just a Map<String,String> is totally correct. The docs do seem to say that, but they also mention that the default value is 'latest'. This suggests that 'latest' is actually a valid value as well and that's not actually a Map. (I suppose you wouldn't typically set it to 'latest' explicitly since that isn't necessary). Still I wonder whether something like this:

    - get: cf-deployment-git
      version: latest

Should be acceptable to the editor (as it is now, with the fix, that will show an error on latest indicating that a 'Map' is expected there).

@srbry

srbry commented Dec 12, 2017

Copy link
Copy Markdown
Contributor Author

@kdvolder you are absolutely correct, I guess its less common to specify latest but you could also specify every. Valid values are: version: ("latest" | "every" | {version}). So I guess we need to support specific strings of latest every and map<String,String>

@kdvolder

Copy link
Copy Markdown
Member

Okay, so I'll take another look at this. We should be able to make it so values 'latest' and 'every' are accepted. Allthough... the docs don't mention 'every' so maybe someone should also try and get the docs fixed if that is correct. Or is that a purposely undocumented feature which is likely to go away in a future release?

@kdvolder

Copy link
Copy Markdown
Member

Actually... doc in other place is more clear about the every value.

https://concourse.ci/get-step.html

But there it is less clear about the fact that version as a map is supposed to be a Map<String,String> (as opposed to something more free-from allowing any kinds of values in the map). I've left it as Map<String,String> as didn't find examples that suggest it can be anything else.

@srbry

srbry commented Dec 14, 2017

Copy link
Copy Markdown
Contributor Author

@kdvolder that is the same bit of documentation I found, I am yet to see a resource that versions outside of Map<String,String> so I think thats a great starting point

@srbry

srbry commented Dec 14, 2017

Copy link
Copy Markdown
Contributor Author

@kdvolder are you sure the latest nightly release included the change? I just installed it and I still get validation errors of: Expecting a 'Version' but found a 'Map'

@kdvolder

Copy link
Copy Markdown
Member

are you sure the latest nightly release included the change?

Well since you ask, one is never 100% sure of anything :-) But I did verify the fix from a snapshot before closing the ticket, so it should be there. Double checking ...

@kdvolder

kdvolder commented Dec 14, 2017

Copy link
Copy Markdown
Member

Just double-checked and it does seem to work fine. To be sure we are using the same thing... I've tried it with the .vsix file snapshot from here:

http://dist.springsource.com/snapshot/STS4/nightly-distributions.html

If that is also what you are using and it doesn't work for you then maybe try this:

  • make sure to reload the page to refresh the links (or you might be getting an old link)
  • make sure to uninstall previous versions of the bundle from vscode before installing a new one (it is actually possible to install several versions at the same time, and I'm not sure what you then actually end up using)
  • make sure to restart vscode after installing new version (vscode will offer to do that for you)

@srbry

srbry commented Dec 14, 2017

Copy link
Copy Markdown
Contributor Author

@kdvolder sorry, complete PICNIC, I somehow had 3 versions installed so had to uninstall them all to get the correct behaviour but this is all working exactly as expected. Thank you for your patience and quick responses :)

@martinlippert martinlippert added this to the 4.0.0.M7 milestone Dec 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants