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

Internal: Change snapshot state for unreleased versions and add validation tests for constants #9228

Merged
merged 1 commit into from Jan 13, 2015

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Jan 9, 2015

Currently the snapshot flag for Version constants is only set to true
for CURRENT. However, this means that the snapshot state changes from
branch to branch. Instead, snapshot should be "is this version
released?". This change also adds a validation test checking that
ID -> constant and vice versa are correct, and fixes one bug found there
(for an unreleased version).

@s1monw
Copy link
Contributor

s1monw commented Jan 9, 2015

ok I think I like this. What I don't like is that we have nothing that detects if this info is accurate when we release. I think what we can do here is extend our release script to get all versions that are marked as snapshots and try to download them ie. check if there is a download for it on github or something like this? we can do this as a followup but I really want this info to be accurate. WDYT

@rjernst
Copy link
Member Author

rjernst commented Jan 9, 2015

Sounds good, I created #9230.

@rjernst
Copy link
Member Author

rjernst commented Jan 12, 2015

@s1monw Any other comments on this?

@jpountz
Copy link
Contributor

jpountz commented Jan 13, 2015

Since the snapshot boolean is taking more importance, maybe we should not make it @Nullable in the constructor? Otherwise LGTM, thanks for opening this other issue about checking this flag when releasing.

@rjernst
Copy link
Member Author

rjernst commented Jan 13, 2015

@jpountz Good point. I changed it from Boolean to boolean. The implication is interacting with an unknown version means it is now false instead of null. But places that check snapshot call snapshot() which returned false when it was null, so it shouldn't behave any different.

@jpountz
Copy link
Contributor

jpountz commented Jan 13, 2015

LGTM

validation tests for constants

Currently the snapshot flag for Version constants is only set to true
for CURRENT.  However, this means that the snapshot state changes from
branch to branch.  Instead, snapshot should be "is this version
released?".  This change also adds a validation test checking that
ID -> constant and vice versa are correct, and fixes one bug found there
(for an unreleased version).
@rjernst rjernst merged commit f241125 into elastic:master Jan 13, 2015
@rjernst rjernst removed the review label Jan 13, 2015
@rjernst rjernst deleted the fix/version-cleanups branch January 21, 2015 23:22
@clintongormley clintongormley added the >test Issues or PRs that are addressing/adding tests label Feb 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>test Issues or PRs that are addressing/adding tests v1.4.3 v1.5.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants