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

Remove ' ' as a configured Confluence space when there's a trailing comma #3090

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

meghanmurphy1
Copy link

@meghanmurphy1 meghanmurphy1 commented Jan 9, 2025

Closes #2643

A fix for when a trailing comma is used as part of 'Confluence space keys' when configuring a Confluence Connector.

Checklists

Pre-Review Checklist

  • this PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check config.yml.example)
  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • if there is no GH issue, please create it. Each PR should have a link to an issue
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)
  • Considered corresponding documentation changes
  • Contributed any configuration settings changes to the configuration reference
  • if you added or changed Rich Configurable Fields for a Native Connector, you made a corresponding PR in Kibana

Changes Requiring Extra Attention

  • Security-related changes (encryption, TLS, SSRF, etc)
  • New external service dependencies added.

Related Pull Requests

Release Note

@meghanmurphy1 meghanmurphy1 changed the title Remove ' ' as a configured Confluence space in the connector when there's a trailing comma Remove ' ' as a configured Confluence space when there's a trailing comma Jan 9, 2025
@meghanmurphy1 meghanmurphy1 self-assigned this Jan 9, 2025
@meghanmurphy1 meghanmurphy1 marked this pull request as ready for review January 9, 2025 18:26
@meghanmurphy1 meghanmurphy1 requested a review from a team as a code owner January 9, 2025 18:26
@meghanmurphy1
Copy link
Author

Went based on where the issue said this problem stemmed from. If there's a way to just strip the whitespace from the string when the configuration is sent that may be better?

@seanstory
Copy link
Member

Great find! I agree that this will address the problem reported in the issue.

Let's add a new test though. While we don't need the test to prove this works (it's simple enough a change that it's pretty obvious it'll work), adding a test will protect us against a regression in case this line/logic is ever broken in the future.

You should be able to add just one pytest function to https://github.com/elastic/connectors/blob/main/tests/sources/test_confluence.py like:

@pytest.mark.asyncio
async def test_fetch_spaces_with_configured_trailing_comma():
  # setup here
  # assertions here

However, I'd bet that this bug goes deeper. Since any "type": "list" configured filed like spaces (see

"spaces": {
"display": "textarea",
"label": "Confluence space keys",
"order": 9,
"tooltip": "This configurable field is ignored when Advanced Sync Rules are used.",
"type": "list",
) is represented by comma-separated strings, this probably means that this bug is lying in wait for other connectors too. I suggest looking into the Field class (see https://github.com/elastic/connectors/blob/main/connectors/source.py#L77) to see if we can eliminate this issue for list-type fields.

@meghanmurphy1
Copy link
Author

ahh ok I see now where the string gets parsed. Thanks!

Copy link
Member

@jedrazb jedrazb left a comment

Choose a reason for hiding this comment

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

LGTM, but would it would be good to have unit tests to ensure this keeps working correctly, I can see _convert unit tests in https://github.com/elastic/connectors/blob/main/tests/test_source.py#L77

@artem-shelkovnikov
Copy link
Member

buildkite test this

@artem-shelkovnikov
Copy link
Member

☝️ Do we need to do changes in backstage so that Meghan/Matt would trigger CI too?

@artem-shelkovnikov
Copy link
Member

Ah sorry, I see you're forking the repository @meghanmurphy1. To make CI run automatically you need to make PRs in this repository, not in forks. I'll fix CI breaking on the above in the meantime

@artem-shelkovnikov
Copy link
Member

CI failure should be fixed by #3095

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

Successfully merging this pull request may close these issues.

Confluence connector fails to sync when list of spaces contains a trailing comma
4 participants