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

Adding fix to allow for command line arguments --cluster-size=3 and … #3757

Merged

Conversation

matsientst
Copy link
Contributor

@matsientst matsientst commented Feb 27, 2023

Fixed: Don't require an equals sign when parsing a command line argument followed by an integer
Added: Improved warning when using an invalid delimiter in gossip seed

Fixed: #3753, #3756

This PR fixes an issue where the command line arguments if followed by an integer throw an ArgumentOutOfRangeException.
When starting the application like:
./src/EventStore.ClusterNode/bin/x64/Release/net6.0/EventStore.ClusterNode --cluster-size 3
The result is an exception:

mattmacchia@Matt-Personal-Mac EventStore % ./src/EventStore.ClusterNode/bin/x64/Release/net6.0/EventStore.ClusterNode --cluster-size 3
[62777, 1,14:34:39.767,FTL] Host terminated unexpectedly.
System.ArgumentOutOfRangeException: Index and length must refer to a location within the string. (Parameter 'length')
   at System.String.Substring(Int32 startIndex, Int32 length)
   at EventStore.Common.Configuration.CommandLineSource.<>c__DisplayClass1_0.<.ctor>g__NormalizeBooleans|1(String x, Int32 i) in /Users/mattmacchia/Documents/dev/eventstore/EventStore/src/EventStore.Common/Configuration/CommandLineSource.cs:line 17
   at System.Linq.Enumerable.SelectIterator[TSource,TResult](IEnumerable`1 source, Func`3 selector)+MoveNext()
   at Microsoft.Extensions.Configuration.CommandLine.CommandLineConfigurationProvider.Load()
   at EventStore.Common.Configuration.CommandLine.Load() in /Users/mattmacchia/Documents/dev/eventstore/EventStore/src/EventStore.Common/Configuration/CommandLine.cs:line 13
   at Microsoft.Extensions.Configuration.ConfigurationRoot..ctor(IList`1 providers)
   at Microsoft.Extensions.Configuration.ConfigurationBuilder.Build()
   at EventStore.Common.Configuration.ConfigurationBuilderExtensions.AddEventStore(IConfigurationBuilder configurationBuilder, String[] args, IDictionary environment, IEnumerable`1 defaultValues) in /Users/mattmacchia/Documents/dev/eventstore/EventStore/src/EventStore.Common/Configuration/ConfigurationBuilderExtensions.cs:line 10
   at EventStore.Core.ClusterVNodeOptions.FromConfiguration(String[] args, IDictionary environment) in /Users/mattmacchia/Documents/dev/eventstore/EventStore/src/EventStore.Core/ClusterVNodeOptions.cs:line 59
   at EventStore.ClusterNode.Program.Main(String[] args) in /Users/mattmacchia/Documents/dev/eventstore/EventStore/src/EventStore.ClusterNode/Program.cs:line 36

This PR also includes a feature to specifically alert you if you're using an invalid delimiter. For example, if you have this in your config:
GossipSeed: nodeb.eventstore.test:2113;nodec.eventstore.test:3113 (invalid delimiter of ;)
This would throw an ArgumentException with a message like:
System.ArgumentException: Invalid delimiter ; for GossipSeed

Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

looks good, for my money this is much easier to read

some comments in line and likely a bug

@matsientst
Copy link
Contributor Author

OK. PR Updated.

@matsientst
Copy link
Contributor Author

@timothycoleman PR updated.

…uster-size 3 and a fix to check for other delimiters besides ,
@timothycoleman timothycoleman force-pushed the matsientst/command-line-params-and-delimiters-for-csv branch from 2b309ea to 535b048 Compare March 2, 2023 17:47
Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

nice 👍

@timothycoleman timothycoleman merged commit ac8abee into master Mar 3, 2023
@timothycoleman timothycoleman deleted the matsientst/command-line-params-and-delimiters-for-csv branch March 3, 2023 13:52
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

🚨 @timothycoleman Failed to create cherry Pick PR due to error:

RequestError [HttpError]: Merge conflict
   at /home/runner/work/_actions/EventStore/Automations/master/cherry-pick-pr-for-label/node_modules/@octokit/request/dist-node/index.js:66:23
   at processTicksAndRejections (internal/process/task_queues.js:97:5) {
 status: 409,
 headers: {
   'access-control-allow-origin': '*',
   'access-control-expose-headers': 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset',
   connection: 'close',
   'content-length': '112',
   'content-security-policy': "default-src 'none'",
   'content-type': 'application/json; charset=utf-8',
   date: 'Fri, 03 Mar 2023 13:52:42 GMT',
   'referrer-policy': 'origin-when-cross-origin, strict-origin-when-cross-origin',
   server: 'GitHub.com',
   'strict-transport-security': 'max-age=31536000; includeSubdomains; preload',
   vary: 'Accept-Encoding, Accept, X-Requested-With',
   'x-content-type-options': 'nosniff',
   'x-frame-options': 'deny',
   'x-github-api-version-selected': '2022-11-28',
   'x-github-media-type': 'github.v3; format=json',
   'x-github-request-id': '700E:649C:12A3C56:2671097:6401FBA9',
   'x-ratelimit-limit': '1000',
   'x-ratelimit-remaining': '990',
   'x-ratelimit-reset': '1677855158',
   'x-ratelimit-resource': 'core',
   'x-ratelimit-used': '10',
   'x-xss-protection': '0'
 },
 request: {
   method: 'POST',
   url: 'https://api.github.com/repos/EventStore/EventStore/merges',
   headers: {
     accept: 'application/vnd.github.v3+json',
     'user-agent': 'octokit-core.js/3.3.2 Node.js/12.22.7 (linux; x64)',
     authorization: 'token [REDACTED]',
     'content-type': 'application/json; charset=utf-8'
   },
   body: '{"base":"cherry-pick-cherry-pick/3757/matsientst/command-line-params-and-delimiters-for-csv-release/oss-v21.10-0bf3221d-2b14-458c-a2f2-cc19db5e3075","commit_message":"Merge 535b048d6266688242a6e8b20bc2f63e36d56e93 into cherry-pick-cherry-pick/3757/matsientst/command-line-params-and-delimiters-for-csv-release/oss-v21.10-0bf3221d-2b14-458c-a2f2-cc19db5e3075 [skip ci]\\n\\n\\nskip-checks: true\\n","head":"535b048d6266688242a6e8b20bc2f63e36d56e93"}',
   request: { agent: [Agent], hook: [Function: bound bound register] }
 },
 documentation_url: 'https://docs.github.com/rest/branches/branches#merge-a-branch'
}

🚨👉 Check https://github.com/EventStore/EventStore/actions/runs/4324063917

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

🚨 @timothycoleman Failed to create cherry Pick PR due to error:

RequestError [HttpError]: Merge conflict
   at /home/runner/work/_actions/EventStore/Automations/master/cherry-pick-pr-for-label/node_modules/@octokit/request/dist-node/index.js:66:23
   at processTicksAndRejections (internal/process/task_queues.js:97:5) {
 status: 409,
 headers: {
   'access-control-allow-origin': '*',
   'access-control-expose-headers': 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset',
   connection: 'close',
   'content-length': '112',
   'content-security-policy': "default-src 'none'",
   'content-type': 'application/json; charset=utf-8',
   date: 'Fri, 03 Mar 2023 13:52:47 GMT',
   'referrer-policy': 'origin-when-cross-origin, strict-origin-when-cross-origin',
   server: 'GitHub.com',
   'strict-transport-security': 'max-age=31536000; includeSubdomains; preload',
   vary: 'Accept-Encoding, Accept, X-Requested-With',
   'x-content-type-options': 'nosniff',
   'x-frame-options': 'deny',
   'x-github-api-version-selected': '2022-11-28',
   'x-github-media-type': 'github.v3; format=json',
   'x-github-request-id': '700D:852A:13E1EED:28F489B:6401FBAF',
   'x-ratelimit-limit': '1000',
   'x-ratelimit-remaining': '978',
   'x-ratelimit-reset': '1677855158',
   'x-ratelimit-resource': 'core',
   'x-ratelimit-used': '22',
   'x-xss-protection': '0'
 },
 request: {
   method: 'POST',
   url: 'https://api.github.com/repos/EventStore/EventStore/merges',
   headers: {
     accept: 'application/vnd.github.v3+json',
     'user-agent': 'octokit-core.js/3.3.2 Node.js/12.22.7 (linux; x64)',
     authorization: 'token [REDACTED]',
     'content-type': 'application/json; charset=utf-8'
   },
   body: '{"base":"cherry-pick-cherry-pick/3757/matsientst/command-line-params-and-delimiters-for-csv-release/oss-v22.10-7a7f70fe-ee6c-4b0a-b3a4-ecc12687fc84","commit_message":"Merge 535b048d6266688242a6e8b20bc2f63e36d56e93 into cherry-pick-cherry-pick/3757/matsientst/command-line-params-and-delimiters-for-csv-release/oss-v22.10-7a7f70fe-ee6c-4b0a-b3a4-ecc12687fc84 [skip ci]\\n\\n\\nskip-checks: true\\n","head":"535b048d6266688242a6e8b20bc2f63e36d56e93"}',
   request: { agent: [Agent], hook: [Function: bound bound register] }
 },
 documentation_url: 'https://docs.github.com/rest/branches/branches#merge-a-branch'
}

🚨👉 Check https://github.com/EventStore/EventStore/actions/runs/4324063917

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

Successfully merging this pull request may close these issues.

Command line arguments with an integer value like --cluster-size 3 not parsing correctly
3 participants