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

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

Merged
merged 1 commit into from
Dec 12, 2017

Conversation

srbry
Copy link
Contributor

@srbry srbry commented Dec 8, 2017

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)) }

@martinlippert
Copy link
Member

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

@kdvolder
Copy link
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
Member

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

@kdvolder
Copy link
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
Copy link
Contributor Author

srbry commented Dec 12, 2017

@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
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
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
Copy link
Contributor Author

srbry commented Dec 14, 2017

@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
Copy link
Contributor Author

srbry commented Dec 14, 2017

@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
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
Copy link
Member

kdvolder commented Dec 14, 2017

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
Copy link
Contributor Author

srbry commented Dec 14, 2017

@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