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

Add MARTI staging dice server properties #2685

Merged
merged 5 commits into from
Dec 15, 2017

Conversation

ssoloff
Copy link
Member

@ssoloff ssoloff commented Dec 11, 2017

In support of #2603.

The new MARTI code has been deployed to the staging server at http://www.tripleawarclub.org/dice-staging/. Before promoting this code to production, I would like to have some volunteers use it for an extended period of time. I thought the easiest way to do this would be to add a new entry to the dice server dropdown on the PBEM/PBF configuration screen. Once merged, I would make an announcement on the forum asking for testing help.

This PR adds a new dice server entry named dice.tripleawarclub.org (BETA) that points to the MARTI staging server. The new entry will appear after the existing MARTI entry but before the internal dice server entry.

However, I'm not sure of the following:

  • Is the (BETA) label sufficient to distinguish the two versions of MARTI? Is there a better term? Should there be more "noise" in the label to make sure users realize it is a beta server (e.g. (***BETA***))?
  • Should the new server only be displayed if the user has the "Enable Beta Features" engine setting enabled? I didn't do this initially because it will require a code change and probably a new property in the .properties file to distinguish beta servers from non-beta servers.

@codecov-io
Copy link

codecov-io commented Dec 11, 2017

Codecov Report

Merging #2685 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #2685      +/-   ##
===========================================
+ Coverage      20.2%   20.2%   +<.01%     
  Complexity     5755    5755              
===========================================
  Files           816     816              
  Lines         73780   73780              
  Branches      12321   12321              
===========================================
+ Hits          14907   14909       +2     
  Misses        56735   56735              
+ Partials       2138    2136       -2
Impacted Files Coverage Δ Complexity Δ
src/main/java/games/strategy/net/nio/Decoder.java 66.34% <0%> (-0.97%) 24% <0%> (-1%)
...tegy/triplea/oddsCalculator/ta/OddsCalculator.java 44.06% <0%> (+0.31%) 13% <0%> (ø) ⬇️
...rategy/triplea/attachments/UnitTypeComparator.java 42.85% <0%> (+7.14%) 11% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06da9c3...085dc30. Read the comment docs.

@panther2
Copy link
Contributor

I suggest

  • to label it dice.tripleawarclub.org (experimental BETA), this should be clear enough
  • not to use the "Enable Beta Features" for it, but to place it at the end of the dice server "list"

@ssoloff
Copy link
Member Author

ssoloff commented Dec 12, 2017

@panther2 Thanks for the naming suggestion.

but to place it at the end of the dice server "list"

This will require a code change as we currently hard-code the "Internal Dice Server" to be the last item in the list. However, I don't think it would be too difficult to give the internal server a custom order in the list so that we can force the beta servers to appear after it. Let me see how that works out.

@ssoloff ssoloff self-assigned this Dec 12, 2017
@RoiEXLab
Copy link
Member

@ssoloff As discussed in triplea-game/tripleawarclub.org#16 , I'd appreciate, if this PR would change the domain to not run into any issues later this year

@ssoloff
Copy link
Member Author

ssoloff commented Dec 12, 2017

@RoiEXLab Yeah, I was thinking the same thing. Might as well do it now. 😄

@panther2
Copy link
Contributor

@ssoloff

as we currently hard-code the "Internal Dice Server" to be the last item in the list

The order

dice.tripleawarclub.org
dice.tripleawarclub.org (experimental BETA)
Internal Dice Roller

would be fine, too, in case it is easier.

@ssoloff ssoloff force-pushed the add-marti-staging-dice-server branch from 903769d to a085673 Compare December 12, 2017 20:18
@ssoloff
Copy link
Member Author

ssoloff commented Dec 12, 2017

@panther2 Sounds good. After looking at the code, it would take a bit more work than I first thought in order to keep the code clean. Not too difficult, but if it's not necessary, I don't want to make the change "just because." If we start getting complaints from users, we can always re-order the list then.

# name to display to the user
name=dice.tripleawarclub.org (experimental BETA)
# dice server host
host=www.tripleawarclub.org
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, we probably want to avoid this domain

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Just so this isn't being missed accidentally, you know when you think about everything else but not this small thing 😄 )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 7ed87cf.

@ssoloff
Copy link
Member Author

ssoloff commented Dec 13, 2017

The MARTI staging server has been moved to the dice-staging.tripleawarclub.org subdomain.

Any thoughts about changing the label displayed in the UI to dice-staging.tripleawarclub.org (experimental BETA)? Or is the hostname an implementation detail that users should not be concerned about?

@ssoloff
Copy link
Member Author

ssoloff commented Dec 13, 2017

@ron-murhammer Do you have any objections to including the staging server entry in the dice server list for general usage? In this PR's current state, the dice server list will appear as

dice.tripleawarclub.org
dice.tripleawarclub.org (experimental BETA)
Internal Dice Roller

@panther2
Copy link
Contributor

@ssoloff Will the use of the staging server require a new e-mail address registration?
If yes, I would suggest to use dice-staging.tripleawarclub.org (experimental BETA). to stronger indicate that it is different.
If no, dice.tripleawarclub.org (experimental BETA) would be just fine.

@RoiEXLab
Copy link
Member

@panther2 AFAIK users do indeed need to reregister themselves

@ssoloff
Copy link
Member Author

ssoloff commented Dec 13, 2017

@panther2 As @RoiEXLab said, users will need to re-register because the staging server uses a separate database. Good point about that fact suggesting the label in the UI be different from the production server.

@prastle
Copy link
Contributor

prastle commented Dec 14, 2017

Off topic but could someone please bump the stable to .7634 which is what all bots use please. @ssoloff @RoiEXLab @DanVanAtta

@ssoloff
Copy link
Member Author

ssoloff commented Dec 15, 2017

@ron-murhammer Bumping... Note that after the latest commit, the dice server list will now appear as

dice.tripleawarclub.org
dice-staging.tripleawarclub.org (experimental BETA)
Internal Dice Roller

Please advise if you have any objections to including the MARTI staging server in the list or, if no objection, if you have any recommendations for improving the label. I'd like to get this out so the QA team can use the staging server in real play. That should give us confidence to promote it into production so we can start fixing the open MARTI issues.

@ron-murhammer
Copy link
Member

@ssoloff Looks fine to me.

@ssoloff ssoloff changed the title Add MARTI staging dice server properties [DO NOT MERGE] Add MARTI staging dice server properties Dec 15, 2017
@ssoloff
Copy link
Member Author

ssoloff commented Dec 15, 2017

Thanks, @ron-murhammer. Going to squash and merge this now.

@ssoloff ssoloff merged commit 6aceff4 into triplea-game:master Dec 15, 2017
@ssoloff ssoloff deleted the add-marti-staging-dice-server branch December 15, 2017 04:52
@ssoloff ssoloff removed their assignment Dec 16, 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.

6 participants