FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

ROX-18155: pg generic store: Upsert by janisz · Pull Request #6882 · stackrox/stackrox · GitHub

/ stackrox Public

ROX-18155: pg generic store: Upsert - #6882

Merged
janisz merged 1 commit into
masterfrom
master-janisz/07-10-ROX-18155_pg_generic_store_Upsert
Jul 13, 2023
ConversationCommits (1)ChecksFiles changed
Merged

ROX-18155: pg generic store: Upsert#6882
janisz merged 1 commit into
masterfrom
master-janisz/07-10-ROX-18155_pg_generic_store_Upsert

Conversation

janisz commented Jul 10, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

The last Method migrated from to generic store. I didn't find a way to make SAC checks generic so I inject them with a function.

Checklist

  • Investigated and inspected CI test results
  • Unit test and regression tests added
  • Evaluated and added CHANGELOG entry if required
  • Determined and documented upgrade steps
  • Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

CI

janisz requested review from a team as code owners July 10, 2023 20:22

ghost commented Jul 10, 2023 •
edited by ghost
Loading

Copy link
Copy Markdown

Images are ready for the commit at 8f100c9.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.1.x-393-g8f100c912a.

janisz force-pushed the master-janisz/07-10-ROX-18155_pg_generic_store_Upsert branch from cf1a456 to cec2bcc Compare July 11, 2023 12:33
janisz requested a review from rhybrillou July 11, 2023 12:40
rhybrillou requested changes Jul 12, 2023
View reviewed changes

rhybrillou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please review the logic behind IsWriteAllowed in pkg/search/postgres/sac.go.
If the function is not needed, it could be removed.
The other changes look good. There are some possible improvement follow-ups.

Comment thread pkg/search/postgres/sac.go Outdated
if err != nil {
return false
}
return sacQuery.GetBaseQuery().GetMatchNoneQuery() != nil

rhybrillou Jul 11, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Write is allowed if the filter query is NOT MatchNoneQuery. I have the feeling this would return true if the sacQuery is a MatchNoneQuery. Am I mistaken here ?

janisz Jul 12, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, I forgot to delete it. It's leftover from my try to make scope checker generic.

Comment thread pkg/search/postgres/store.go Outdated

func (s *GenericStore[T, PT]) upsert(ctx context.Context, objs ...PT) error {
if s.insertInto == nil {
err := errors.New("invalid operation, missing insertInto function")

rhybrillou Jul 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: Why not making this error a file var or const ?

Comment thread pkg/search/postgres/store.go Outdated

func (s *GenericStore[T, PT]) copyFrom(ctx context.Context, objs ...PT) error {
if s.copyFromObj == nil {
err := errors.New("invalid operation, missing copyFromObj function")

rhybrillou Jul 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: Why not making this error a file var or const ?

Comment thread pkg/search/postgres/store.go Outdated
}

if err := s.copyFromObj(ctx, s, tx, objs...); err != nil {
if err := tx.Rollback(ctx); err != nil {

rhybrillou Jul 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: could we differentiate the rollback error from the copyFromObj one ?

Comment thread pkg/search/postgres/store.go Outdated
return err
}

conn, err := s.AcquireConn(ctx, ops.Get)

rhybrillou Jul 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Although it was already so in the generated store function, I think it would make sense to use ops.Upsert here.
Can be tracked as a follow-up.

Comment thread pkg/search/postgres/store.go Outdated
utils.Should(err)
return err
}
conn, err := s.AcquireConn(ctx, ops.Get)

rhybrillou Jul 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Although it was already so in the generated store function, I think it would make sense to use ops.Upsert here.
Can be tracked as a follow-up.

Comment thread pkg/search/postgres/store.go Outdated
return s.permissionChecker != nil
}

func (s *GenericStore[T, PT]) permissionCheckerUpsertAllows(ctx context.Context) error {

rhybrillou Jul 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: naming - how about permissionCheckerAllowsUpsert ?

Comment thread pkg/search/postgres/store.go
janisz force-pushed the master-janisz/07-10-ROX-18155_pg_generic_store_Upsert branch from cec2bcc to f943032 Compare July 12, 2023 11:03
This was referenced Jul 12, 2023
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged

janisz commented Jul 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

This was referenced Jul 12, 2023
Merged
Merged
janisz requested a review from rhybrillou July 12, 2023 16:55
janisz force-pushed the master-janisz/07-10-ROX-18155_pg_generic_store_Upsert branch from f943032 to 5e857e5 Compare July 12, 2023 16:56
janisz force-pushed the master-janisz/07-10-refactor_pg_remove_blank_lines branch from 7215e6a to bc74488 Compare July 12, 2023 16:59
janisz force-pushed the master-janisz/07-10-ROX-18155_pg_generic_store_Upsert branch from 5e857e5 to f9f38c6 Compare July 12, 2023 16:59
janisz force-pushed the master-janisz/07-10-refactor_pg_remove_blank_lines branch from bc74488 to 33d8992 Compare July 13, 2023 08:40
Base automatically changed from master-janisz/07-10-refactor_pg_remove_blank_lines to master July 13, 2023 09:38
janisz force-pushed the master-janisz/07-10-ROX-18155_pg_generic_store_Upsert branch from f9f38c6 to 8f100c9 Compare July 13, 2023 10:32
janisz requested a review from a team July 13, 2023 11:35
This was referenced Jul 13, 2023
Closed
Merged

janisz commented Jul 13, 2023

Copy link
Copy Markdown
Contributor Author

/retest

rhybrillou approved these changes Jul 13, 2023
View reviewed changes
Comment thread pkg/search/postgres/store.go
janisz merged commit 04ad904 into master Jul 13, 2023
janisz deleted the master-janisz/07-10-ROX-18155_pg_generic_store_Upsert branch July 13, 2023 16:39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

rhybrillou rhybrillou approved these changes

Assignees

No one assigned

Labels

area/central area/postgres

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL