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

fix: fixes a regex problem on groundswell manifest #9

Merged
merged 22 commits into from
Oct 15, 2024

Conversation

justinegeffen
Copy link
Contributor

Copy link

netlify bot commented Apr 2, 2024

Deploy Preview for seqera-docs ready!

Name Link
🔨 Latest commit a77a645
🔍 Latest deploy log https://app.netlify.com/sites/seqera-docs/deploys/670e7c55c13a060009a433e0
😎 Deploy Preview https://deploy-preview-9--seqera-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@justinegeffen justinegeffen added the 2. Dev/PM review Needs a review by a Dev or PM label Apr 5, 2024
@justinegeffen
Copy link
Contributor Author

@gwright99, please review this when you have a chance. Thank you!

@gwright99
Copy link

I'm a bit foggy on this issue (looking at original ticket now - I remember the issue, I just don't remember exactly how to test) -- any suggestions on how your folks tested it?

@justinegeffen
Copy link
Contributor Author

@bebosudo, would you be open to reviewing this? Thank you!

@justinegeffen
Copy link
Contributor Author

I'm a bit foggy on this issue (looking at original ticket now - I remember the issue, I just don't remember exactly how to test) -- any suggestions on how your folks tested it?

I've added @bebosudo to this issue for that feedback as I believe he tested it. :)

Copy link
Member

@bebosudo bebosudo left a comment

Choose a reason for hiding this comment

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

Unfortunately the proposed fix doesn't work when the tower db url contains extra parameters (like 99% of the cases) and only works with a db url without parameters like for the customer who pointed it out (see also this?), as I showed in this message:

[alberto@fedora]$ podman run --rm -it  --entrypoint bash mysql:5.7
bash-4.2#
bash-4.2# TOWER_DB_URL="jdbc:mysql://tower.cluster-c1rjv6umdpvw.eu-west-2.rds.amazonaws.com:3306/tower?&usePipelineAuth=false&useBatchMultiSend=false"
bash-4.2# mysqlsh --uri "$(echo $TOWER_DB_URL |sed -e 's@jdbc:\(.*\)@\1@')"
Invalid URI: Invalid connection option ''.

bash-4.2# TOWER_DB_URL="jdbc:mysql://tower.cluster-c1rjv6umdpvw.eu-west-2.rds.amazonaws.com:3306/tower"
bash-4.2# mysqlsh --uri "$(echo $TOWER_DB_URL |sed -e 's@jdbc:\(.*\)@\1@')"
Please provide the password for '[email protected]:3306': 

That's because mysqlsh doesn't accept extra parameters.

I proposed a fix that should work for both cases, your feedback would be appreciated @swampie @gwright99

bash-4.2# TOWER_DB_URL="jdbc:mysql://tower.cluster-c1rjv6umdpvw.eu-west-2.rds.amazonaws.com:3306/tower?&usePipelineAuth=false&useBatchMultiSend=false" 
bash-4.2# mysqlsh --uri "$(echo $TOWER_DB_URL |cut -d'?' -f1 |sed -e 's@jdbc:\(.*\)@\1@')"
Please provide the password for '[email protected]:3306':

bash-4.2# TOWER_DB_URL="jdbc:mysql://tower.cluster-c1rjv6umdpvw.eu-west-2.rds.amazonaws.com:3306/tower"
bash-4.2# mysqlsh --uri "$(echo $TOWER_DB_URL |cut -d'?' -f1 |sed -e 's@jdbc:\(.*\)@\1@')"
Please provide the password for '[email protected]:3306': 

@gwright99
Copy link

Given the recent addition of a mandatory connection string modifier for v24.1+ (and all the various connection string modifiers that need to be used for various DB products < 24.1), I think we need to scrap the old way and go with @bebosudo's new approach.

justinegeffen and others added 3 commits August 5, 2024 23:07
…8s/groundswell.yml

Co-authored-by: Alberto Chiusole <[email protected]>
Signed-off-by: Justine Geffen <[email protected]>
…8s/groundswell.yml

Co-authored-by: Alberto Chiusole <[email protected]>
Signed-off-by: Justine Geffen <[email protected]>
@justinegeffen
Copy link
Contributor Author

Thanks, @bebosudo! Much appreciated. @gwright99, please review/approve when you have a chance. Thank you!

…8s/groundswell.yml to platform_versioned_docs/version-23.3/enterprise_templates/k8s/groundswell.yml

Signed-off-by: Justine Geffen <[email protected]>
…8s/groundswell.yml to platform_versioned_docs/version-23.4/enterprise_templates/k8s/groundswell.yml

Signed-off-by: Justine Geffen <[email protected]>
@justinegeffen justinegeffen removed the request for review from swampie August 7, 2024 17:22
@justinegeffen justinegeffen dismissed bebosudo’s stale review August 7, 2024 17:23

Changes have been implemented

@justinegeffen
Copy link
Contributor Author

@bebosudo, following up on this for approval. Thank you! :)

Copy link
Member

@bebosudo bebosudo left a comment

Choose a reason for hiding this comment

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

the regex looks correct now, I didn't test it

non-blocking: left some comments about possible improvements we could do, don't know if you prefer to follow up in a separate PR, up to you

justinegeffen and others added 4 commits October 10, 2024 11:09
Co-authored-by: Alberto Chiusole <[email protected]>
Signed-off-by: Justine Geffen <[email protected]>
Signed-off-by: Justine Geffen <[email protected]>
Updated groundswell manifest 24.1

Signed-off-by: Justine Geffen <[email protected]>
@justinegeffen justinegeffen added 4. Reviews complete Reviews complete. Remove label when confirmed in prod. and removed 2. Dev/PM review Needs a review by a Dev or PM labels Oct 10, 2024
@justinegeffen
Copy link
Contributor Author

Thanks, @bebosudo! I updated this for 24.1 as well.

@justinegeffen justinegeffen added 1. Editor review Needs a language review and removed 4. Reviews complete Reviews complete. Remove label when confirmed in prod. labels Oct 10, 2024
@justinegeffen justinegeffen merged commit bded849 into master Oct 15, 2024
6 checks passed
@justinegeffen justinegeffen deleted the groundswell-regex branch October 15, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. Editor review Needs a language review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants