Skip to content

fix(kafka topic): update regexp for topic name#663

Merged
craicoverflow merged 4 commits intomainfrom
topic_regexp
May 25, 2021
Merged

fix(kafka topic): update regexp for topic name#663
craicoverflow merged 4 commits intomainfrom
topic_regexp

Conversation

@rkpattnaik780
Copy link
Contributor

@rkpattnaik780 rkpattnaik780 commented May 17, 2021

Closes #654

Verification Steps

  1. Run the command to interactively create a kafka topic
go run ./cmd/rhoas kafka topic create
  1. kafka.k should be valid and accepted whereas . and .. should be rejected.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation change
  • Other (please specify)

Checklist

  • Documentation added for the feature
  • CI and all relevant tests are passing
  • Code Review completed
  • Verified independently by reviewer

@rkpattnaik780 rkpattnaik780 marked this pull request as ready for review May 24, 2021 15:30
@rkpattnaik780 rkpattnaik780 changed the title WIP: fix: update regexp for topic name fix(kafka topic): update regexp for topic name May 24, 2021
Copy link
Contributor

@craicoverflow craicoverflow left a comment

Choose a reason for hiding this comment

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

A couple of small nits which would make small improvements to UX (mainly the error message) but nothing major. Love the tests!


const (
legalNameChars = "^[a-zA-Z0-9\\_\\-]+$"
legalNameChars = `[a-zA-Z0-9._-]`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
legalNameChars = `[a-zA-Z0-9._-]`
legalNameChars = "[a-zA-Z0-9._-]"

}

if (name == ".") || (name == "..") {
return fmt.Errorf(`invalid topic name "%v"; can't be "." or ".."`, name)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to repeat the topic name is we know it is either "." or ".." already.

Suggested change
return fmt.Errorf(`invalid topic name "%v"; can't be "." or ".."`, name)
return errors.New(`topic name can not be "." or ".."`)

@craicoverflow craicoverflow merged commit f00da5a into main May 25, 2021
@craicoverflow craicoverflow deleted the topic_regexp branch May 25, 2021 13:32
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.

Topic names should allow a full-stop (".")

2 participants