CNF-14938: implement GET /O2ims_infrastructureMonitoring/v1/alarms/{alarmEventRecordId} #319

pixelsoccupied · 2024-11-18T22:02:22Z

This PR implements alarms/{alarmEventRecordId} where you query the DB for existing record using a given UUID

// 200

{
    "alarmAcknowledged": false,
    "alarmDefinitionId": "c7e5e1e6-6f16-4b9a-807f-c18088def392",
    "alarmEventRecordId": "c1a66f8c-8454-4e3b-8d8d-9cbe119f200a",
    "alarmRaisedTime": "2024-11-19T15:25:30.485991-05:00",
    "extensions": null,
    "perceivedSeverity": 3,
    "probableCauseId": "2266f5fd-2dca-47bf-a701-802c463162c8",
    "resourceTypeID": "550e8400-e29b-41d4-a716-446655440000"
}

// 404

{
    "additionalAttributes": {
        "UUID": "0b07e1b1-3473-458a-9ce3-03911c2f581d"
    },
    "detail": "Could not find AlarmEventRecord for given UUID",
    "status": 404
}

// 400

{
    "detail": "Invalid format for parameter alarmEventRecordId: error unmarshaling 0b07e1b1-3473-458a-9ce3-03911c2f text as *uuid.UUID: invalid UUID format",
    "status": 400
}

// 500

{
    "detail": "failed to get AlarmEventRecord due to issues with DB conn: failed to call database: failed to connect to `host=localhost user=alarms database=alarms`: dial error (dial tcp 127.0.0.1:5432: connect: connection refused)",
    "status": 500
}

openshift-ci-robot · 2024-11-18T22:02:26Z

@pixelsoccupied: This pull request references CNF-14938 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

tbd

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2024-11-18T22:02:27Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2024-11-19T20:36:12Z

@pixelsoccupied: This pull request references CNF-14938 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

This PR implements alarms/{alarmEventRecordId} where you query the DB for existing record using a given UUID

// 200

{
   "alarmAcknowledged": false,
   "alarmDefinitionId": "c7e5e1e6-6f16-4b9a-807f-c18088def392",
   "alarmEventRecordId": "c1a66f8c-8454-4e3b-8d8d-9cbe119f200a",
   "alarmRaisedTime": "2024-11-19T15:25:30.485991-05:00",
   "extensions": null,
   "perceivedSeverity": 3,
   "probableCauseId": "2266f5fd-2dca-47bf-a701-802c463162c8",
   "resourceTypeID": "550e8400-e29b-41d4-a716-446655440000"
}

// 404

{
   "additionalAttributes": {
       "UUID": "0b07e1b1-3473-458a-9ce3-03911c2f581d"
   },
   "detail": "Could not find AlarmEventRecord for given UUID",
   "status": 404
}

// 400

{
   "detail": "Invalid format for parameter alarmEventRecordId: error unmarshaling 0b07e1b1-3473-458a-9ce3-03911c2f text as *uuid.UUID: invalid UUID format",
   "status": 400
}

// 500

{
   "detail": "failed to get AlarmEventRecord due to issues with DB conn: failed to call database: failed to connect to `host=localhost user=alarms database=alarms`: dial error (dial tcp 127.0.0.1:5432: connect: connection refused)",
   "status": 500
}

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

pixelsoccupied · 2024-11-19T20:43:34Z

/test

openshift-ci · 2024-11-19T20:43:40Z

@pixelsoccupied: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test ci-bundle-operator-bundle
/test ci-job
/test images
/test scorecard

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-kni-oran-o2ims-main-ci-bundle-operator-bundle
pull-ci-openshift-kni-oran-o2ims-main-ci-job
pull-ci-openshift-kni-oran-o2ims-main-images

In response to this:

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

pixelsoccupied · 2024-11-19T20:43:59Z

/test all

pixelsoccupied · 2024-11-19T22:07:34Z

/test all

pixelsoccupied · 2024-11-19T22:29:03Z

/cc @alegacy

pixelsoccupied · 2024-11-19T22:30:11Z

will revisit the testing since testcontainers was pulling in too much dependency. Most likely look into kind cluster like TALM + something in release repo

alegacy · 2024-11-19T23:20:02Z

internal/service/alarms/internal/db/postgres.go

 	// TODO: update config with trace, timeouts etc.
-	poolConfig, err := pgxpool.ParseConfig(fmt.Sprintf("postgres://%s:%s@%s:%s/%s",
+	poolConfig, err := pgxpool.ParseConfig(fmt.Sprintf("postgres://%s:%s@%s:%s/%s?sslmode=disable",


We should try to ensure that sslmode gets enabled here.

alegacy · 2024-11-19T23:21:42Z

internal/service/alarms/internal/db/repo/alarms_repository.go

+`
+
+	// Run query
+	rows, _ := ar.Db.Query(ctx, query, uuid.String()) // note: err is passed on to Collect* func so we can ignore this


I assume the answer is yes, but are we sure that the Query method protects us against SQL injection attacks? ... i know in this particular case it isn't possible since the UUID string would be validated for format before this point, but in other instances that may not always be the case.

Yeah basically using $ syntax should prevent any injection issues! The author talks about it here a bit more. There's also a helper function called NamedArgs (not used here) is also safe.

alegacy · 2024-11-19T23:24:15Z

/lgtm

mlguerrero12 · 2024-11-20T14:13:19Z

internal/service/alarms/internal/db/repo/alarms_repository.go

+func (ar *AlarmsRepository) GetAlarmEventRecordWithUuid(ctx context.Context, uuid uuid.UUID) ([]models.AlarmEventRecord, error) {
+	// Build sql query
+	query := `
+SELECT alarm_event_record_id, alarm_definition_id, probable_cause_id, alarm_raised_time, 


I think it's worth having a look at
https://github.com/doug-martin/goqu or squirrel

Ah would be nice to have a type safe code. I only experimented with sqlc (similar to goqu but takes sql and coverts to typed code) but was generating too much extra code. But this seems nice it it's simply wrapping the usual sql code with a type safety - so no random syntax errors. Does this work well with pgx? I suppose the query is simply converted to a string and then pgx simply continues as usual?

Let's experiment with it a bit more but +1 type safety. @alegacy

mlguerrero12 · 2024-11-20T14:15:43Z

internal/service/alarms/internal/alarms_server.go

 )

 type AlarmsServer struct {
-	AlarmsRepository *AlarmsRepository
+	AlarmsRepository *repo.AlarmsRepository


you can mock this to have some basic unit tests

Yeah absolutely but there's not much to test with this current PR (other than actually sql stuff) so not creating any interface for now to mock. Will update this on a follow up PR where we have stuff outside of the sql calls to test...wdyt?

mlguerrero12 · 2024-11-20T16:45:24Z

/approve

openshift-ci · 2024-11-20T16:45:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mlguerrero12

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mlguerrero12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ecordId}`

mlguerrero12 · 2024-11-20T16:54:38Z

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2024

pixelsoccupied force-pushed the alarm-with-uuid branch from 73158a0 to f5973b5 Compare November 19, 2024 20:37

pixelsoccupied marked this pull request as ready for review November 19, 2024 22:28

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 19, 2024

openshift-ci bot requested review from browsell and mlguerrero12 November 19, 2024 22:28

openshift-ci bot requested a review from alegacy November 19, 2024 22:29

alegacy reviewed Nov 19, 2024

View reviewed changes

openshift-ci bot assigned alegacy Nov 19, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2024

mlguerrero12 reviewed Nov 20, 2024

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2024

pixelsoccupied added 3 commits November 20, 2024 11:50

implement `GET /O2ims_infrastructureMonitoring/v1/alarms/{alarmEventR…

d61d826

…ecordId}`

remove testcontainers dependency

8265568

remove testcontainers dependency

f468a90

pixelsoccupied force-pushed the alarm-with-uuid branch from 9ce5e3c to f468a90 Compare November 20, 2024 16:51

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2024

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2024

openshift-ci bot assigned mlguerrero12 Nov 20, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNF-14938: implement GET /O2ims_infrastructureMonitoring/v1/alarms/{alarmEventRecordId} #319

CNF-14938: implement GET /O2ims_infrastructureMonitoring/v1/alarms/{alarmEventRecordId} #319

pixelsoccupied commented Nov 18, 2024 •

edited

Loading

openshift-ci-robot commented Nov 18, 2024 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Nov 18, 2024

openshift-ci-robot commented Nov 19, 2024 •

edited by openshift-ci bot

Loading

pixelsoccupied commented Nov 19, 2024

openshift-ci bot commented Nov 19, 2024

pixelsoccupied commented Nov 19, 2024

pixelsoccupied commented Nov 19, 2024

pixelsoccupied commented Nov 19, 2024

pixelsoccupied commented Nov 19, 2024 •

edited

Loading

alegacy Nov 19, 2024

alegacy Nov 19, 2024

pixelsoccupied Nov 20, 2024

alegacy commented Nov 19, 2024

mlguerrero12 Nov 20, 2024

pixelsoccupied Nov 20, 2024

mlguerrero12 Nov 20, 2024

pixelsoccupied Nov 20, 2024

mlguerrero12 commented Nov 20, 2024

openshift-ci bot commented Nov 20, 2024

mlguerrero12 commented Nov 20, 2024

CNF-14938: implement GET /O2ims_infrastructureMonitoring/v1/alarms/{alarmEventRecordId} #319

Are you sure you want to change the base?

CNF-14938: implement GET /O2ims_infrastructureMonitoring/v1/alarms/{alarmEventRecordId} #319

Conversation

pixelsoccupied commented Nov 18, 2024 • edited Loading

openshift-ci-robot commented Nov 18, 2024 • edited by openshift-ci bot Loading

openshift-ci bot commented Nov 18, 2024

openshift-ci-robot commented Nov 19, 2024 • edited by openshift-ci bot Loading

pixelsoccupied commented Nov 19, 2024

openshift-ci bot commented Nov 19, 2024

pixelsoccupied commented Nov 19, 2024

pixelsoccupied commented Nov 19, 2024

pixelsoccupied commented Nov 19, 2024

pixelsoccupied commented Nov 19, 2024 • edited Loading

alegacy Nov 19, 2024

Choose a reason for hiding this comment

alegacy Nov 19, 2024

Choose a reason for hiding this comment

pixelsoccupied Nov 20, 2024

Choose a reason for hiding this comment

alegacy commented Nov 19, 2024

mlguerrero12 Nov 20, 2024

Choose a reason for hiding this comment

pixelsoccupied Nov 20, 2024

Choose a reason for hiding this comment

mlguerrero12 Nov 20, 2024

Choose a reason for hiding this comment

pixelsoccupied Nov 20, 2024

Choose a reason for hiding this comment

mlguerrero12 commented Nov 20, 2024

openshift-ci bot commented Nov 20, 2024

mlguerrero12 commented Nov 20, 2024

pixelsoccupied commented Nov 18, 2024 •

edited

Loading

openshift-ci-robot commented Nov 18, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 19, 2024 •

edited by openshift-ci bot

Loading

pixelsoccupied commented Nov 19, 2024 •

edited

Loading