Skip to content

Conversation

@Lewih
Copy link
Contributor

@Lewih Lewih commented Apr 7, 2023

I tested this with ncat and a small config file as

site_configuration = {
    'logging': [
        {
        'handlers_perflog': [

            {
            'type': 'httpjson',
            'url': 'http://localhost:12345',
            'level': 'info',
            'extras': {
                'facility': 'reframe',
                'data-version': '1.0'
            },
            'debug': False,
            'extra_headers': {
               'ApiKey': 'test'
            }
            }
            ]
        }
    ]
}

Closes #2850.

Should we implement a test ?

@jenkins-cscs
Copy link
Collaborator

Can I test this patch?

@teojgo teojgo requested review from teojgo and vkarak April 7, 2023 13:45
Copy link
Contributor

@teojgo teojgo left a comment

Choose a reason for hiding this comment

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

You would need to add this to the configuration schema as well.

@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2023

Codecov Report

Patch coverage: 20.00% and project coverage change: -0.05 ⚠️

Comparison is base (2811360) 86.93% compared to head (20b67e6) 86.89%.

❗ Current head 20b67e6 differs from pull request most recent head dd29444. Consider uploading reports for the commit dd29444 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2854      +/-   ##
===========================================
- Coverage    86.93%   86.89%   -0.05%     
===========================================
  Files           60       60              
  Lines        11505    11500       -5     
===========================================
- Hits         10002     9993       -9     
- Misses        1503     1507       +4     
Impacted Files Coverage Δ
reframe/core/logging.py 80.28% <20.00%> (-0.40%) ⬇️

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Lewih
Copy link
Contributor Author

Lewih commented Apr 7, 2023

You would need to add this to the configuration schema as well.

gonna add it as "extra_headers": {"type": "object"},

@Lewih
Copy link
Contributor Author

Lewih commented Apr 7, 2023

Documentation should be updated as well

@Lewih Lewih changed the title HTTPJSONHandler http POST customisable header #2850 [feat]HTTPJSONHandler http POST customisable header #2850 Apr 7, 2023
@vkarak vkarak changed the title [feat]HTTPJSONHandler http POST customisable header #2850 [feat] HTTPJSONHandler http POST customisable header #2850 Apr 7, 2023
@Lewih Lewih requested a review from teojgo April 8, 2023 16:47
@Lewih
Copy link
Contributor Author

Lewih commented Apr 11, 2023

@teojgo , The PR now follows the community guidelines and the schema has been updated.

As far as I have seen, no test is included for extras and the others entries of httpjson handler. I would postpone a more complete test implementation for the handler in another PR.

@teojgo
Copy link
Contributor

teojgo commented Apr 11, 2023

@teojgo , The PR now follows the community guidelines and the schema has been updated.

As far as I have seen, no test is included for extras and the others entries of httpjson handler. I would postpone a more complete test implementation for the handler in another PR.

@Lewih Thanks for the PR and the update. Yes, we don't have any testing for the handler at the moment but suggestions are always welcome.

@Lewih
Copy link
Contributor Author

Lewih commented Apr 11, 2023

@Lewih Thanks for the PR and the update. Yes, we don't have any testing for the handler at the moment but suggestions are always welcome.

I opened an issue #2858 in order to track this.

@Lewih
Copy link
Contributor Author

Lewih commented Apr 11, 2023

Synched with upstream branch in order to re-run the test to make sure nothing breaks, then PR should be able to merge.

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

Lgtm; I just did a couple of minor style enhancements.

@vkarak vkarak added this to the ReFrame 4.2 milestone Apr 14, 2023
@vkarak vkarak changed the title [feat] HTTPJSONHandler http POST customisable header #2850 [feat] Allow custom HTTP headers with the httpjson log handler Apr 14, 2023
@vkarak
Copy link
Contributor

vkarak commented Apr 14, 2023

We'll merge this after #2861 is addressed, which has broken our CI.

@vkarak vkarak merged commit cc422fc into reframe-hpc:develop Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

HTTPJSONHandler http POST customisable header

5 participants