Skip to content

fix: optimize profile_by_app_group_name to query only active AppGroups#308

Open
ce314007 wants to merge 3 commits into
BaritoLog:masterfrom
ce314007:fix_profile_app_name
Open

fix: optimize profile_by_app_group_name to query only active AppGroups#308
ce314007 wants to merge 3 commits into
BaritoLog:masterfrom
ce314007:fix_profile_app_name

Conversation

@ce314007
Copy link
Copy Markdown
Contributor

No description provided.

)

if @app_group.blank? || !@app_group.ACTIVE?
if @app_group.blank? || @app_group.status != "ACTIVE"
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.

we don't need the @app_group.status != "ACTIVE" since it already filter on find_by process.

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.

ack,

@helm_infrastructure = @app_group.helm_infrastructure_in_default_location
@helm_infrastructure = @app_group.helm_infrastructures.active.first unless @helm_infrastructure.present?

if @helm_infrastructure.blank?
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.

do we need this check inside the present? since it will never be called.

)

if @app_group.blank? || !@app_group.ACTIVE?
if @app_group.blank? || @app_group.status != "ACTIVE"
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.

similar from above related to status != "ACTIVE"

}, status: :not_found) && return
end

if @helm_infrastructure.blank?
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.

I don't think we need this right? where @helm_infrastructure defined?

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.

in my case when testing in local, its happen if there are appgroups that have provisioning status PENDING and status INACTIVE, . if i remove this, it will throw a random html response if i hit the appgroup that have condition like that.

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.

what's this random html? is it 404 or 500? if it is 404, I guess it is expected because it is still not active, wdyt @fadlinurhasan

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.

its 404, make sense, will update it

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.

3 participants